-
Notifications
You must be signed in to change notification settings - Fork 1
Enhanced context format #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added ContextProcessorManager to manage multiple context processors. - Introduced various processors: ArrayProcessor, DateTimeProcessor, FallbackProcessor, NullProcessor, ObjectProcessor, ResourceProcessor, ScalarProcessor, StringableProcessor, and ThrowableProcessor. - Enhanced RpcLogger to utilize ContextProcessorManager for processing log context data.
- Implemented unit tests for various context processors including ArrayProcessor, DateTimeProcessor, FallbackProcessor, NullProcessor, ObjectProcessor, ResourceProcessor, ScalarProcessor, StringableProcessor, and ThrowableProcessor. - Ensured coverage for processing different data types and edge cases, validating the functionality of each processor. - Enhanced the RpcLoggerTest with additional tests for logging with complex contexts and custom processors.
|
Warning Rate limit exceeded@roxblnfk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a context-normalization pipeline and object processors; RpcLogger now accepts an optional processor, uses it to normalize log contexts before delegating to AppLogger. Multiple object processors, comprehensive unit tests, and a composer test-script change were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RpcLogger
participant Processor as Context Processor
participant AppLogger
Note over RpcLogger,Processor: Processor = custom callable or DefaultProcessor::createDefault()
Client->>RpcLogger: log(level, message, context)
RpcLogger->>Processor: invoke processor on context
Processor-->>RpcLogger: processedContext
RpcLogger->>AppLogger: log(level, message, processedContext)
AppLogger-->>RpcLogger: ack
RpcLogger-->>Client: return
rect rgba(230,240,255,0.45)
Note right of Processor: Object handling (high-level):\n- resources -> "<type> resource"\n- arrays -> recursive processing\n- objects -> first matching ObjectProcessor\n- fallback -> @class + public props
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Nitpick comments (26)
src/ContextProcessor/ThrowableProcessor.php (2)
27-37: Leverage the recursiveProcessor to serialize chained exceptions (and silence PHPMD).Using the callback to process the previous throwable makes output richer and resolves the unused-parameter warning.
Apply:
- public function process(mixed $value, callable $recursiveProcessor): mixed - { - return [ + public function process(mixed $value, callable $recursiveProcessor): mixed + { + $previous = $value->getPrevious(); + return [ 'class' => \get_class($value), 'message' => $value->getMessage(), 'code' => $value->getCode(), 'file' => $value->getFile(), 'line' => $value->getLine(), - 'trace' => $value->getTraceAsString(), + 'trace' => $value->getTraceAsString(), + 'previous' => $previous !== null ? $recursiveProcessor($previous) : null, ]; }If you prefer not to change the payload shape right now, minimally suppress PHPMD on this method:
/** * @param \Throwable $value * @param callable(mixed): mixed $recursiveProcessor * @return array<string, mixed> + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */
35-35: Consider emitting structured stack frames (optional).Arrays from getTrace() are more machine-friendly than a single string. If backward compatibility allows, expose frames (e.g., as traceFrames) alongside trace.
src/ContextProcessor/DateTimeProcessor.php (2)
22-30: Silence PHPMD for the intentionally unused recursiveProcessor.This processor doesn’t recurse; suppress the warning locally.
/** * @param \DateTimeInterface $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { return $value->format(\DateTimeInterface::ATOM); }
29-29: Optional: Use RFC3339_EXTENDED to retain sub-second precision.If higher precision timestamps are useful for log correlation, prefer RFC3339_EXTENDED.
- return $value->format(\DateTimeInterface::ATOM); + return $value->format(\DateTimeInterface::RFC3339_EXTENDED);src/ContextProcessor/ResourceProcessor.php (2)
21-29: Suppress PHPMD for the unused recursiveProcessor (no recursion needed here)./** * @param resource $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { return \get_resource_type($value) . ' resource'; }
26-29: Confirming PHP >= 8.1: get_resource_id() is availablecomposer.json specifies PHP >= 8.1, so get_resource_id (added in PHP 7.2) can be used safely. Including the resource ID can improve debugability.
Locations to update:
- src/ContextProcessor/ResourceProcessor.php (process method)
Proposed diff:
--- a/src/ContextProcessor/ResourceProcessor.php +++ b/src/ContextProcessor/ResourceProcessor.php @@ public function process(mixed $value, callable $recursiveProcessor): mixed - return \get_resource_type($value) . ' resource'; + $type = \get_resource_type($value); + $id = \get_resource_id($value); + return \sprintf('%s resource (%d)', $type, $id);src/ContextProcessor/NullProcessor.php (1)
27-31: Return the given value and suppress the unused callback warning.Returning $value expresses pass-through and eliminates one unused-parameter warning; suppress the other.
/** * @param null $value * @param callable(mixed): mixed $recursiveProcessor * @return null + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { - // Null values are already suitable for logging - return null; + // Null values are already suitable for logging + return $value; }src/ContextProcessor/FallbackProcessor.php (2)
17-21: Suppress PHPMD for intentionally unused parameter in canProcess
$valueis intentionally unused because this processor is the last-resort handler. Suppress the warning to keep static analysis clean.Apply this diff:
- public function canProcess(mixed $value): bool + /** + * @SuppressWarnings("UnusedFormalParameter") + */ + public function canProcess(mixed $value): bool { // This processor can handle anything as a last resort return true; }
23-30: Prefer get_debug_type and suppress PHPMD for unused$recursiveProcessor
- Suppress the unused-parameter warning for
$recursiveProcessor.- Consider
\get_debug_type($value)for more informative type names (e.g., class names for objects, “float” instead of “double”). Not functionally required given upstream processors, but nicer as a final fallback.Apply this diff:
- /** - * @param callable(mixed): mixed $recursiveProcessor - * @return string - */ + /** + * @param callable(mixed): mixed $recursiveProcessor + * @return string + * @SuppressWarnings("UnusedFormalParameter") + */ public function process(mixed $value, callable $recursiveProcessor): mixed { - return \gettype($value); + return \get_debug_type($value); }src/ContextProcessor/ScalarProcessor.php (1)
23-33: Suppress PHPMD for intentionally unused$recursiveProcessor
processreturns the scalar as-is and doesn’t need the recursive callback. Suppress the warning to avoid noise from static analysis.Apply this diff:
/** * @param scalar $value * @param callable(mixed): mixed $recursiveProcessor * @return scalar */ - public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings("UnusedFormalParameter") + */ + public function process(mixed $value, callable $recursiveProcessor): mixed { // Scalar values are already suitable for logging return $value; }src/ContextProcessor/StringableProcessor.php (1)
21-29: Suppress PHPMD for intentionally unused$recursiveProcessorThe recursive callback isn’t needed here since we directly cast to string. Suppress the warning.
Apply this diff:
/** * @param \Stringable $value * @param callable(mixed): mixed $recursiveProcessor * @return string */ - public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings("UnusedFormalParameter") + */ + public function process(mixed $value, callable $recursiveProcessor): mixed { return (string) $value; }tests/Unit/ContextProcessor/ArrayProcessorTest.php (2)
17-28: Avoid leaving open resources in data providersUsing
fopen('php://memory', 'r')creates a resource that isn’t closed. Prefertmpfile(), which is cleaned up automatically by PHP.Apply this diff:
return [ 'string' => ['not an array'], 'integer' => [42], 'float' => [3.14], 'boolean' => [true], 'null' => [null], 'object' => [new \stdClass()], - 'resource' => [\fopen('php://memory', 'r')], + 'resource' => [\tmpfile()], ];
75-99: Strengthen the nested-array test and remove the unused$expectedvariable warningCurrently,
$expectedis unused and the assertion only checks for a key. Make the recursive processor transform arrays so we can assert the exact expected result.Apply this diff:
// Mock recursive processor that adds a prefix to strings - $recursiveProcessor = static function ($value) { - return \is_string($value) ? 'processed:' . $value : $value; - }; + $recursiveProcessor = static function ($value) { + if (\is_array($value)) { + return 'processed:' . \serialize($value); + } + return \is_string($value) ? 'processed:' . $value : $value; + }; $result = $this->processor->process($array, $recursiveProcessor); $expected = [ 'level1' => 'processed:' . \serialize(['level2' => ['value' => 'deep']]), ]; - // Since the recursive processor is called on the nested array, - // we need to test that it was called correctly - $this->assertArrayHasKey('level1', $result); + // The nested array should be transformed by the recursive processor + $this->assertSame($expected, $result);tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
17-28: Avoid leaking resources in data providersThe
nonObjectProvideropens a stream resource that is never closed. Data providers are executed before the test and you don’t get a chance tofclosethere, which can lead to handle leaks across the test run.Apply this minimal change inside the provider and move the resource case to a dedicated test where you can open/close explicitly:
return [ 'string' => ['not an object'], 'integer' => [42], 'float' => [3.14], 'boolean' => [true], 'null' => [null], 'array' => [[]], - 'resource' => [\fopen('php://memory', 'r')], + // resource case covered in a dedicated test to avoid leaking open handles ];Then add this method elsewhere in the class to cover the resource case:
public function testCannotProcessResource(): void { $h = \fopen('php://memory', 'r'); try { $this->assertFalse($this->processor->canProcess($h)); } finally { \fclose($h); } }tests/Unit/ContextProcessor/NullProcessorTest.php (1)
17-29: Prevent resource leaks in providerAs with the other test, the resource created in the data provider is never closed.
Remove the resource from the provider:
return [ 'string' => ['test'], 'integer' => [42], 'float' => [3.14], 'boolean true' => [true], 'boolean false' => [false], 'array' => [[]], 'object' => [new \stdClass()], - 'resource' => [\fopen('php://memory', 'r')], + // resource case is covered by a dedicated test to ensure proper closing ];Then add a dedicated test method where you can open and close:
public function testCannotProcessResource(): void { $h = \fopen('php://memory', 'r'); try { $this->assertFalse($this->processor->canProcess($h)); } finally { \fclose($h); } }tests/Unit/ContextProcessor/StringableProcessorTest.php (1)
17-29: Close the opened resource from the data providerThe provider opens a memory stream (Line 27) that is never closed in the test, leaking a resource. Close it in the test via a finally block.
#[DataProvider('nonStringableProvider')] public function testCannotProcessNonStringable(mixed $value): void { - $this->assertFalse($this->processor->canProcess($value)); + try { + $this->assertFalse($this->processor->canProcess($value)); + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }Also applies to: 43-47
tests/Unit/ContextProcessor/ScalarProcessorTest.php (2)
17-30: Close resource from data provider in canProcess testThe data provider opens a resource (Line 28) that isn’t closed, causing a leak when used in this test. Close it in a finally block.
#[DataProvider('scalarValuesProvider')] public function testCanProcessScalarValues(mixed $value, bool $expected): void { - $this->assertSame($expected, $this->processor->canProcess($value)); + try { + $this->assertSame($expected, $this->processor->canProcess($value)); + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }Also applies to: 32-36
17-30: Also close resource in process testSame leak exists when the dataset is used in testProcessScalarValues. Close in a finally block.
#[DataProvider('scalarValuesProvider')] public function testProcessScalarValues(mixed $value, bool $canProcess): void { - if ($canProcess) { - $recursiveProcessor = static fn($v) => $v; - $result = $this->processor->process($value, $recursiveProcessor); - $this->assertSame($value, $result); - } else { - $this->addToAssertionCount(1); // Skip non-scalar values - } + try { + if ($canProcess) { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($value, $recursiveProcessor); + $this->assertSame($value, $result); + } else { + $this->addToAssertionCount(1); // Skip non-scalar values + } + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }Also applies to: 38-48
tests/Unit/ContextProcessor/ResourceProcessorTest.php (1)
69-88: Rename and simplify: not actually testing cURL resourceOn PHP 8+, curl_init returns CurlHandle (object), and this test ultimately processes a stream resource, not cURL. The extension gate and curl handle are unnecessary. Rename the test and drop cURL code.
- public function testProcessCurlResource(): void - { - if (!\extension_loaded('curl')) { - $this->markTestSkipped('cURL extension not available'); - } - - $curlHandle = \curl_init(); - - // In PHP 8+, curl_init returns CurlHandle object, not resource - // Let's test with a different resource type that's consistently a resource - $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - \curl_close($curlHandle); - - $this->assertSame('stream resource', $result); - } + public function testProcessDataStreamResource(): void + { + $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + \fclose($resource); + $this->assertSame('stream resource', $result); + }src/ContextProcessor/ContextProcessorManager.php (1)
65-75: Optional: add recursion depth guard to prevent runaway processingSelf-referential arrays/objects can cause infinite recursion. Adding a max-depth cap is a pragmatic safety net.
- public function processValue(mixed $value): mixed + private int $maxDepth = 10; + + public function processValue(mixed $value, int $depth = 0): mixed { + if ($depth >= $this->maxDepth) { + return '*max-depth*'; + } foreach ($this->processors as $processor) { if ($processor->canProcess($value)) { - return $processor->process($value, [$this, 'processValue']); + $recursive = function (mixed $v) use ($depth) { + return $this->processValue($v, $depth + 1); + }; + return $processor->process($value, $recursive); } } // This should never happen due to FallbackProcessor, but just in case return \gettype($value); }Note: No changes needed where processValue is called—existing calls will start at depth 0.
tests/Unit/ContextProcessor/FallbackProcessorTest.php (2)
17-41: Prefer deterministic cleanup over register_shutdown_function for test-created resources.Using register_shutdown_function to close the data-provider resource works, but it’s global and can hide leaks. Consider closing the handle in a class-level tearDownAfterClass or creating/closing the resource within individual tests that need it to keep lifecycle explicit.
Example approach using tearDownAfterClass:
private static $resource; public static function allTypesProvider(): array { self::$resource = \fopen('php://memory', 'r'); return [ // ... 'resource' => [self::$resource, 'resource'], ]; } public static function tearDownAfterClass(): void { if (\is_resource(self::$resource)) { \fclose(self::$resource); } }
59-70: Assert that the closed resource is actually closed to avoid false confidence.Minor robustness: after fclose($resource), add an assertion that is_resource($resource) is false (where applicable) before proceeding, to make the test intent explicit. It won’t harm environments where PHP still reports it as a resource, since you’re only asserting the result type is a string.
- \fclose($resource); + \fclose($resource); + // Depending on PHP version, this may be false or still report as resource. + $this->assertIsString($this->processor->process($resource, static fn($v) => $v));tests/Unit/RpcLoggerTest.php (4)
546-559: Silence PHPMD warnings for unused $recursiveProcessor in test stubs.These stubs don’t use the recursive processor; make that explicit to satisfy static analysis.
Apply this diff:
public function process(mixed $value, callable $recursiveProcessor): mixed { + unset($recursiveProcessor); // unused in this test stub // Mask email for privacy $parts = \explode('@', $value); return \substr($parts[0], 0, 2) . '***@' . $parts[1]; }
586-601: Silence PHPMD warnings for unused $recursiveProcessor in URL processor stub.public function process(mixed $value, callable $recursiveProcessor): mixed { + unset($recursiveProcessor); // unused in this test stub $parsed = \parse_url($value); return [ 'scheme' => $parsed['scheme'] ?? null, 'host' => $parsed['host'] ?? null, 'path' => $parsed['path'] ?? null, ]; }
604-614: Silence PHPMD warnings for unused $recursiveProcessor in CC processor stub.public function process(mixed $value, callable $recursiveProcessor): mixed { + unset($recursiveProcessor); // unused in this test stub return '****-****-****-' . \substr($value, -4); }
485-503: Ensure resource creation succeeds to avoid undefined behavior on fopen failure.fopen() may return false; guard the test to fail fast or skip if the resource can’t be created.
- $resource = \fopen('php://memory', 'r'); + $resource = \fopen('php://memory', 'r'); + $this->assertIsResource($resource, 'Failed to create test resource');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
src/ContextProcessor/ArrayProcessor.php(1 hunks)src/ContextProcessor/ContextProcessorInterface.php(1 hunks)src/ContextProcessor/ContextProcessorManager.php(1 hunks)src/ContextProcessor/DateTimeProcessor.php(1 hunks)src/ContextProcessor/FallbackProcessor.php(1 hunks)src/ContextProcessor/NullProcessor.php(1 hunks)src/ContextProcessor/ObjectProcessor.php(1 hunks)src/ContextProcessor/ResourceProcessor.php(1 hunks)src/ContextProcessor/ScalarProcessor.php(1 hunks)src/ContextProcessor/StringableProcessor.php(1 hunks)src/ContextProcessor/ThrowableProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/ContextProcessor/ArrayProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ContextProcessorManagerTest.php(1 hunks)tests/Unit/ContextProcessor/DateTimeProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/FallbackProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/NullProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ObjectProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ResourceProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ScalarProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/StringableProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ThrowableProcessorTest.php(1 hunks)tests/Unit/RpcLoggerTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (23)
src/ContextProcessor/FallbackProcessor.php (1)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)
src/ContextProcessor/ResourceProcessor.php (5)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/FallbackProcessor.php (2)
canProcess(17-21)process(27-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)src/ContextProcessor/ScalarProcessor.php (2)
canProcess(18-21)process(28-32)
tests/Unit/RpcLoggerTest.php (4)
src/ContextProcessor/ContextProcessorManager.php (2)
ContextProcessorManager(13-104)addProcessor(27-30)src/RpcLogger.php (2)
log(35-59)RpcLogger(15-60)tests/Unit/RpcSpy.php (4)
getCallCount(42-45)getLastCall(37-40)RpcSpy(17-51)reset(47-50)tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (6)
__toString(69-72)__toString(151-154)__toString(170-173)canProcess(125-128)process(130-133)testProcessorOrdering(147-163)
src/ContextProcessor/ObjectProcessor.php (6)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/FallbackProcessor.php (2)
canProcess(17-21)process(27-30)src/ContextProcessor/ThrowableProcessor.php (2)
canProcess(17-20)process(27-37)src/ContextProcessor/StringableProcessor.php (2)
canProcess(16-19)process(26-29)src/ContextProcessor/DateTimeProcessor.php (2)
canProcess(17-20)process(27-30)
src/ContextProcessor/ScalarProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/NullProcessor.php (2)
canProcess(17-20)process(27-31)
tests/Unit/ContextProcessor/ArrayProcessorTest.php (1)
src/ContextProcessor/ArrayProcessor.php (1)
ArrayProcessor(14-42)
src/ContextProcessor/DateTimeProcessor.php (1)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)
src/ContextProcessor/ArrayProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)
tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
src/ContextProcessor/ObjectProcessor.php (1)
ObjectProcessor(15-49)
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (1)
src/ContextProcessor/ThrowableProcessor.php (1)
ThrowableProcessor(15-38)
src/ContextProcessor/StringableProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (1)
src/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(13-104)processContext(40-60)addProcessor(27-30)
src/ContextProcessor/ContextProcessorInterface.php (9)
src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/FallbackProcessor.php (2)
canProcess(17-21)process(27-30)src/ContextProcessor/NullProcessor.php (2)
canProcess(17-20)process(27-31)src/ContextProcessor/ResourceProcessor.php (2)
canProcess(16-19)process(26-29)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)src/ContextProcessor/ScalarProcessor.php (2)
canProcess(18-21)process(28-32)src/ContextProcessor/ThrowableProcessor.php (2)
canProcess(17-20)process(27-37)src/ContextProcessor/StringableProcessor.php (2)
canProcess(16-19)process(26-29)src/ContextProcessor/DateTimeProcessor.php (2)
canProcess(17-20)process(27-30)
src/ContextProcessor/ContextProcessorManager.php (11)
src/RpcLogger.php (1)
__construct(22-26)src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (3)
canProcess(16-19)process(26-41)ArrayProcessor(14-42)src/ContextProcessor/FallbackProcessor.php (3)
canProcess(17-21)process(27-30)FallbackProcessor(15-31)src/ContextProcessor/NullProcessor.php (3)
canProcess(17-20)process(27-31)NullProcessor(15-32)src/ContextProcessor/ResourceProcessor.php (3)
canProcess(16-19)process(26-29)ResourceProcessor(14-30)src/ContextProcessor/ObjectProcessor.php (3)
canProcess(17-20)process(27-48)ObjectProcessor(15-49)src/ContextProcessor/ScalarProcessor.php (3)
canProcess(18-21)process(28-32)ScalarProcessor(16-33)src/ContextProcessor/ThrowableProcessor.php (3)
canProcess(17-20)process(27-37)ThrowableProcessor(15-38)src/ContextProcessor/StringableProcessor.php (3)
canProcess(16-19)process(26-29)StringableProcessor(14-30)src/ContextProcessor/DateTimeProcessor.php (3)
canProcess(17-20)process(27-30)DateTimeProcessor(15-31)
tests/Unit/ContextProcessor/DateTimeProcessorTest.php (2)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (3)
canProcess(125-128)testProcessDateTime(40-48)process(130-133)src/ContextProcessor/DateTimeProcessor.php (1)
DateTimeProcessor(15-31)
tests/Unit/ContextProcessor/ScalarProcessorTest.php (3)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (5)
CoversClass(12-208)canProcess(125-128)testProcessScalarValues(25-38)process(130-133)setUp(204-207)tests/Unit/ContextProcessor/NullProcessorTest.php (3)
CoversClass(12-54)DataProvider(36-40)setUp(50-53)src/ContextProcessor/ScalarProcessor.php (1)
ScalarProcessor(16-33)
tests/Unit/ContextProcessor/StringableProcessorTest.php (1)
src/ContextProcessor/StringableProcessor.php (1)
StringableProcessor(14-30)
tests/Unit/ContextProcessor/NullProcessorTest.php (2)
tests/Unit/ContextProcessor/ScalarProcessorTest.php (4)
CoversClass(12-54)DataProvider(32-36)DataProvider(38-48)setUp(50-53)src/ContextProcessor/NullProcessor.php (1)
NullProcessor(15-32)
tests/Unit/ContextProcessor/FallbackProcessorTest.php (1)
src/ContextProcessor/FallbackProcessor.php (1)
FallbackProcessor(15-31)
tests/Unit/ContextProcessor/ResourceProcessorTest.php (1)
src/ContextProcessor/ResourceProcessor.php (1)
ResourceProcessor(14-30)
src/ContextProcessor/ThrowableProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)
src/RpcLogger.php (1)
src/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(13-104)__construct(18-21)processContext(40-60)
src/ContextProcessor/NullProcessor.php (3)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/ScalarProcessor.php (2)
canProcess(18-21)process(28-32)
🪛 PHPMD (2.15.0)
src/ContextProcessor/FallbackProcessor.php
17-17: Avoid unused parameters such as '$value'. (Unused Code Rules)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/ResourceProcessor.php
26-26: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/RpcLoggerTest.php
468-468: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
553-553: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
610-610: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
677-677: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/ScalarProcessor.php
28-28: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ArrayProcessorTest.php
92-92: Avoid unused local variables such as '$expected'. (Unused Code Rules)
(UnusedLocalVariable)
src/ContextProcessor/DateTimeProcessor.php
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ObjectProcessorTest.php
48-48: Avoid unused private fields such as '$secret'. (Unused Code Rules)
(UnusedPrivateField)
64-64: Avoid unused private fields such as '$private'. (Unused Code Rules)
(UnusedPrivateField)
src/ContextProcessor/StringableProcessor.php
26-26: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php
113-113: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
130-130: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/ThrowableProcessor.php
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/NullProcessor.php
27-27: Avoid unused parameters such as '$value'. (Unused Code Rules)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (24)
src/ContextProcessor/ThrowableProcessor.php (1)
17-20: Ordering Verified: ThrowableProcessor precedes ObjectProcessor
- In src/ContextProcessor/ContextProcessorManager.php,
addProcessor(new ThrowableProcessor())is called on line 91, and
addProcessor(new ObjectProcessor())on line 99.- This ensures throwables are handled before generic objects.
No changes needed.
src/ContextProcessor/ContextProcessorInterface.php (1)
16-31: Interface design looks solid.Clear contract, generic-friendly PHPDoc, and callable signature are appropriate for recursive processing.
src/RpcLogger.php (1)
45-47: Sanity check: processing empty contexts is cheap and safeGood use of ContextProcessorManager; it returns an empty array for empty input, so this call is safe on the hot path.
tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
42-61: Thorough coverage and correct expectations for public-vs-nonpublic properties and recursionTests accurately validate:
- Only public properties are serialized.
- Private/protected are excluded.
- stdClass handling (both non-empty and empty).
- Nested array/object values are delegated to the provided recursive processor.
- Mixed falsy values are preserved.
Also applies to: 76-161
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (1)
17-203: LGTM: solid end-to-end coverage of the default pipeline and processor orderingThe suite exercises null/scalars, DateTime, Throwable, Stringable, arrays, resources, objects, ordering, and a complex mixed context. Assertions align with the implementations and the default registration order in ContextProcessorManager.
tests/Unit/ContextProcessor/NullProcessorTest.php (1)
31-48: LGTM: behavior and boundaries are correct
- canProcess(null) returns true; non-nulls are rejected.
- process returns null and does not touch the recursive processor.
tests/Unit/ContextProcessor/DateTimeProcessorTest.php (1)
16-54: LGTM: correct coverage for DateTimeInterface handlingTests verify both DateTime and DateTimeImmutable, negative cases, and stable formatting with concrete offsets. Expectations match the production implementation.
src/ContextProcessor/ObjectProcessor.php (1)
17-21: Appropriate type checkHandling all objects here is correct; specialized processors (Throwable, DateTime, Stringable) precede this one in the manager’s registration order.
tests/Unit/ContextProcessor/StringableProcessorTest.php (4)
12-16: Good coverage and setupCoversClass is correctly targeted and the setup is clear.
49-61: LGTM: Conversion behavior aligns with implementationThe test validates that process returns (string)$value and matches StringableProcessor’s contract.
64-79: LGTM: Complex stringable logic coveredCovers non-trivial __toString logic and expected uppercase result.
81-94: LGTM: Empty string case coveredEdge case for empty string is appropriately asserted.
tests/Unit/ContextProcessor/ScalarProcessorTest.php (1)
12-16: Solid structure and coverage targetCoversClass annotation and setup look good.
tests/Unit/ContextProcessor/ResourceProcessorTest.php (4)
30-35: LGTM: canProcess for resourceOpens and closes the resource correctly; assertion is appropriate.
43-53: LGTM: Stream resource processingAsserts the formatted string precisely and handles cleanup.
55-67: LGTM: File resource processingUses temp file safely, cleans up, and validates expected output.
90-105: LGTM: Closed resource handlingGood defensive branching across PHP versions—only processes when still recognized as resource.
src/ContextProcessor/ArrayProcessor.php (1)
14-41: Array processing is correct and preserves keysImplementation cleanly delegates recursion and maintains keys. Psalm annotations are appropriate.
src/ContextProcessor/ContextProcessorManager.php (2)
53-57: Potential key collision when casting keys to stringCasting context keys to string may overwrite entries if both 1 and '1' exist, causing silent data loss. Consider detecting collisions or documenting this behavior.
Would you like a small guard that appends suffixes on collision (e.g., "1#2") or a debug log to flag it?
81-103: Default processor order is sensiblePriority given to Null, Scalar, specific object types, then collections/resources, then generic object and fallback. This ordering will produce predictable results.
tests/Unit/ContextProcessor/FallbackProcessorTest.php (1)
12-16: Good coverage and intent are clear.Tests validate both canProcess() and process() paths and include edge cases (e.g., closed resource). Nicely scoped.
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (1)
52-69: Solid assertions for Throwable normalization.Ensures class, message, code, and basic location/trace information are present with correct types. Looks good.
tests/Unit/RpcLoggerTest.php (2)
1-58: Core RpcLogger behavior tests look solid.Level normalization, method routing, and empty-context behavior are consistently validated. Using RpcSpy to assert single-call behavior and payload shape is pragmatic.
Also applies to: 64-102, 121-157, 168-208, 209-240, 242-337, 339-357
159-166: Enum import is unnecessary
TheLogLevelEnumis declared inRoadRunner\PsrLogger\Tests\Unit, the same namespace asRpcLoggerTest, so referencingLogLevelEnum::Warningwithout an import is valid.
| PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), | ||
| PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), | ||
| PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), | ||
| 'log' => $this->logger->log((string) $message, $processedContext), | ||
| PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), | ||
| default => throw new PsrInvalidArgumentException("Invalid log level `$normalizedLevel` provided."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential TypeError: cast Stringable messages to string for all logger calls
AppLogger methods likely accept string messages. Passing a \Stringable without casting can throw a TypeError with strict types. Some branches already cast; others don’t.
Apply this diff to consistently cast:
- PsrLogLevel::ERROR => $this->logger->error($message, $processedContext),
- PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext),
- PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext),
- 'log' => $this->logger->log((string) $message, $processedContext),
- PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext),
+ PsrLogLevel::ERROR => $this->logger->error((string) $message, $processedContext),
+ PsrLogLevel::WARNING => $this->logger->warning((string) $message, $processedContext),
+ PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext),
+ 'log' => $this->logger->log((string) $message, $processedContext),
+ PsrLogLevel::DEBUG => $this->logger->debug((string) $message, $processedContext),Optional follow-up: cast once after processing context to reduce repetition.
// Process context data for structured logging using the processor manager
$processedContext = $this->contextProcessor->processContext($context);
+ $message = (string) $message;Then remove per-branch casts.
🤖 Prompt for AI Agents
In src/RpcLogger.php around lines 52 to 57, some logger calls pass a \Stringable
$message without casting which can cause a TypeError under strict types; ensure
the message is always a string by casting before use — either replace each
logger call to use (string) $message consistently in every branch or better:
after processing the context, assign $msg = (string) $message and use $msg in
all switch branches (then remove any per-branch casts).
| public function testProcessCustomException(): void | ||
| { | ||
| $customException = new class('Custom message', 999) extends \Exception {}; | ||
| $recursiveProcessor = static fn($v) => $v; | ||
|
|
||
| $result = $this->processor->process($customException, $recursiveProcessor); | ||
|
|
||
| $this->assertIsArray($result); | ||
| $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous')); | ||
| $this->assertSame('Custom message', $result['message']); | ||
| $this->assertSame(999, $result['code']); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix assertion for anonymous exception class naming.
get_class() for an anonymous class extending Exception returns something like "class@anonymous..."; it does not contain "Exception@anonymous". This assertion will be brittle or fail across PHP versions.
Apply this change:
- $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous'));
+ $this->assertStringContainsString('@anonymous', $result['class']);🤖 Prompt for AI Agents
In tests/Unit/ContextProcessor/ThrowableProcessorTest.php around lines 84 to 95,
the assertion expecting 'Exception@anonymous' is brittle because get_class() for
an anonymous class returns a name like "class@anonymous..." (not necessarily
containing the parent class name). Update the assertion to check for the generic
'@anonymous' marker (e.g., assert that $result['class'] contains '@anonymous' or
use a regex to match /@anonymous/), leaving other assertions unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. I'm thinking about how we can optimize this. The approach with processors is quite flexible, but we need a very high-performance logger because I plan to use it in Temporal Workflows. I think we should simplify some things for better performance.
- We should move the processors to the Internal folder and mark all classes as
@internal. This way, we can redesign them in the future without breaking backward compatibility. - I think we should handle built-in types (like null, string, int, float, bool, array) in one processor or even directly in the manager. We need processors only for objects of specific classes, right?
Anw, what do you think if we allow adding specific processors in user view? |
That's why builtin types might be processed right in the manager. |
… scalar, and resource handling
…TypeProcessor - Removed ArrayProcessor, NullProcessor, and ScalarProcessor. - Introduced BuiltInTypeProcessor to handle null, scalar, and array types efficiently. - Updated ContextProcessorManager to register BuiltInTypeProcessor. - Added unit tests for BuiltInTypeProcessor to ensure correct functionality.
|
@roxblnfk I have updated them, pls confirm again. tysm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (6)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (1)
81-95: Add regression tests for cyclic references to prevent infinite recursionEchoing the earlier note: arrays/objects can self‑reference and will currently recurse unbounded. Add tests for self‑referential arrays and mutually‑referencing objects; then implement a recursion guard in the manager (seen-set + max depth).
I can provide a ready-to-merge guard for ContextProcessorManager plus tests if you’d like.
src/RpcLogger.php (1)
52-56: Potential TypeError: cast Stringable messages to string consistently (duplicate of prior review)Some branches pass $message as \Stringable to AppLogger, which likely expects string. This can throw under strict types. Cast once and reuse to avoid per-branch discrepancies.
Apply this diff to cast once and remove per-branch casts:
// Process context data for structured logging using the processor manager $processedContext = $this->contextProcessor->processContext($context); + $message = (string) $message; match ($normalizedLevel) { PsrLogLevel::EMERGENCY, PsrLogLevel::ALERT, PsrLogLevel::CRITICAL, - PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), - PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), - PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), - 'log' => $this->logger->log((string) $message, $processedContext), - PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), + PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), + PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), + PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info($message, $processedContext), + 'log' => $this->logger->log($message, $processedContext), + PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), };tests/Unit/ContextProcessor/ThrowableProcessorTest.php (1)
84-95: Fix brittle assertion for anonymous exception class name.get_class() for an anonymous class produces a name like "class@anonymous..." (parent class name is not guaranteed to be present). Assert the generic marker instead of "Exception@anonymous".
Apply:
- $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous')); + $this->assertStringContainsString('@anonymous', $result['class']);tests/Unit/RpcLoggerTest.php (3)
544-581: Custom processor precedence is ineffective; BuiltInTypeProcessor will preempt strings. Prepend or assert processed output.ContextProcessorManager registers BuiltInTypeProcessor first; addProcessor() appends. Your email processor won’t run for strings because BuiltInTypeProcessor short-circuits first. Also, the test doesn’t assert that masking actually occurred.
Recommended changes:
- Introduce a prepend API on ContextProcessorManager and use it in tests.
- Assert the processed context by invoking $processorManager->processContext($context) directly (in addition to the existing RPC assertions).
Patch the test:
- // Create processor manager with custom processor added first - $processorManager = new ContextProcessorManager(); - $processorManager->addProcessor($emailProcessor); + // Create processor manager and prepend the custom processor so it runs before defaults + $processorManager = new ContextProcessorManager(); + $processorManager->addProcessorFirst($emailProcessor); // Create logger with custom processor manager $logger = new RpcLogger($this->appLogger, $processorManager); $context = [ 'user_email' => '[email protected]', 'admin_email' => '[email protected]', 'regular_string' => 'not an email', 'user_id' => 123, ]; + // Assert transformation happened at the manager level + $processed = $processorManager->processContext($context); + $this->assertSame('jo***@example.com', $processed['user_email']); + $this->assertSame('ad***@company.org', $processed['admin_email']); + $this->assertSame('not an email', $processed['regular_string']); + $this->assertSame(123, $processed['user_id']); + $logger->info('User action performed', $context);Suppress PHPMD “UnusedFormalParameter” for anonymous processor’s process() method:
- public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function process(mixed $value, callable $recursiveProcessor): mixed {Manager addition (outside this file):
--- a/src/Internal/ContextProcessor/ContextProcessorManager.php +++ b/src/Internal/ContextProcessor/ContextProcessorManager.php @@ class ContextProcessorManager public function addProcessor(ContextProcessorInterface $processor): void { $this->processors[] = $processor; } + + /** + * Prepend a processor so it runs before defaults. + */ + public function addProcessorFirst(ContextProcessorInterface $processor): void + { + \array_unshift($this->processors, $processor); + }Quick verification script to confirm current precedence:
#!/bin/bash # Expectation: BuiltInTypeProcessor is registered before others, and addProcessor appends. rg -nC2 'class ContextProcessorManager|registerDefaultProcessors|function\s+addProcessor\(' srcAlso applies to: 561-564, 575-581
583-635: Multiple custom processors have the same precedence issue; also not asserting actual transformation.URL and CC processors are appended after defaults, so strings never reach them. Strengthen the test by prepending processors and asserting processed shapes via the manager.
Apply:
- $processorManager = new ContextProcessorManager(); - $processorManager->addProcessor($urlProcessor); - $processorManager->addProcessor($ccProcessor); + $processorManager = new ContextProcessorManager(); + $processorManager->addProcessorFirst($urlProcessor); + $processorManager->addProcessorFirst($ccProcessor);Add explicit assertions:
- $logger = new RpcLogger($this->appLogger, $processorManager); + $logger = new RpcLogger($this->appLogger, $processorManager); $context = [ 'website' => 'https://example.com/path/to/resource', 'payment_card' => '1234-5678-9012-3456', 'regular_data' => 'normal string', 'amount' => 99.99, ]; + $processed = $processorManager->processContext($context); + $this->assertSame( + ['scheme' => 'https', 'host' => 'example.com', 'path' => '/path/to/resource'], + $processed['website'] + ); + $this->assertSame('****-****-****-3456', $processed['payment_card']); + $this->assertSame('normal string', $processed['regular_data']); + $this->assertSame(99.99, $processed['amount']);Suppress PHPMD in the two anonymous processors:
- public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function process(mixed $value, callable $recursiveProcessor): mixed {(repeat for the credit-card processor)
Also applies to: 616-619, 629-635
656-699: Ordering test doesn’t validate ordering due to BuiltInTypeProcessor handling ints first. Prepend and assert via manager.As written, your int processors won’t run. Prepend them and assert that the first one wins by inspecting the manager’s output.
Apply:
- $processorManager = new ContextProcessorManager(); - $processorManager->addProcessor($firstProcessor); // Added first, should be used - $processorManager->addProcessor($secondProcessor); // Added second, should be skipped + $processorManager = new ContextProcessorManager(); + $processorManager->addProcessorFirst($firstProcessor); // Prepend to run before BuiltInTypeProcessor + $processorManager->addProcessorFirst($secondProcessor); // Still after $firstProcessorAssert ordering effect explicitly:
- $context = ['number' => 42]; + $context = ['number' => 42]; + $processed = $processorManager->processContext($context); + $this->assertSame('first:42', $processed['number']); $logger->debug('Ordering test', $context);Suppress PHPMD in both anonymous processors:
- public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function process(mixed $value, callable $recursiveProcessor): mixed {Also applies to: 683-686, 689-699
🧹 Nitpick comments (26)
composer.json (1)
46-46: Split coverage run into a separate script to keepcomposer testfast and xdebug-agnosticAlways enabling Xdebug slows local/CI runs and fails if Xdebug isn’t installed. Recommend keeping
testlean and adding a dedicatedtest:coveragescript.Apply:
"scripts": { "cs:diff": "php-cs-fixer fix --dry-run -v --diff --show-progress dots", "cs:fix": "php-cs-fixer fix -v", - "test": "XDEBUG_MODE=coverage phpunit", + "test": "phpunit", + "test:coverage": "php -d xdebug.mode=coverage phpunit", "psalm": "psalm" },src/Internal/ContextProcessor/StringableProcessor.php (2)
16-16: Make the classfinalsince it’s internalPrevents unintended extension and allows future internal changes without BC guarantees.
-class StringableProcessor implements ContextProcessorInterface +final class StringableProcessor implements ContextProcessorInterface
23-31: Suppress PHPMD false-positive for unused$recursiveProcessorSignature is mandated by the interface; this processor doesn’t need recursion. Suppress the warning locally.
/** * @param \Stringable $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings("UnusedFormalParameter") */ public function process(mixed $value, callable $recursiveProcessor): mixed { return (string) $value; }src/Internal/ContextProcessor/DateTimeProcessor.php (3)
17-17: Considerfinalfor internal classSame rationale as other internal processors.
-class DateTimeProcessor implements ContextProcessorInterface +final class DateTimeProcessor implements ContextProcessorInterface
24-32: Suppress PHPMD unused-parameter warning on$recursiveProcessorProcessor doesn’t recurse; keep interface but silence the false-positive.
/** * @param \DateTimeInterface $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings("UnusedFormalParameter") */ public function process(mixed $value, callable $recursiveProcessor): mixed { return $value->format(\DateTimeInterface::ATOM); }
29-32: Optional: include microseconds for higher-resolution logsIf you want sub-second precision, consider RFC3339_EXTENDED.
- return $value->format(\DateTimeInterface::ATOM); + return $value->format(\DateTimeInterface::RFC3339_EXTENDED);If compatibility with existing consumers relies on ATOM, keep current behavior.
src/Internal/ContextProcessor/ObjectProcessor.php (2)
17-17: Mark asfinalInternal utility that shouldn’t be extended externally.
-class ObjectProcessor implements ContextProcessorInterface +final class ObjectProcessor implements ContextProcessorInterface
48-50: Fallback to class name is reasonable; consider JsonSerializable laterReturning class name when no public properties exist is a sensible default. As a future enhancement, you might special-case JsonSerializable to serialize its payload, if present.
src/Internal/ContextProcessor/ResourceProcessor.php (3)
16-16: Make classfinalConsistent with other internal processors; avoids accidental inheritance.
-class ResourceProcessor implements ContextProcessorInterface +final class ResourceProcessor implements ContextProcessorInterface
23-31: Suppress PHPMD unused-parameter warningSame interface constraint; suppress locally.
/** * @param resource $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings("UnusedFormalParameter") */ public function process(mixed $value, callable $recursiveProcessor): mixed { return \get_resource_type($value) . ' resource'; }
30-31: Nit: unify resource string format (optional)Some teams prefer
resource(type)for quick scanning. Purely stylistic; current output is fine.- return \get_resource_type($value) . ' resource'; + return 'resource(' . \get_resource_type($value) . ')';src/Internal/ContextProcessor/ContextProcessorInterface.php (1)
25-32: Nit: tighten the contract around the recursive callback and behaviorConsider documenting behavioral guarantees to guide implementors and help static analyzers:
- State that processors should be side‑effect free and avoid throwing (or document expected exceptions).
- Name and describe the recursion callback as a stable “processValue” contract to make intent explicit.
Example doc enhancement:
/** * Process the value and return a serializable representation. * * @param TValue $value The value to process - * @param callable(mixed): mixed $recursiveProcessor Function to process nested values recursively + * @param callable(mixed): mixed $recursiveProcessor Recursor provided by the manager; + * must be used to process nested values. Implementations should be side‑effect free + * and avoid throwing; return a best‑effort representation instead. * @return TProcessed Processed value suitable for logging */src/Internal/ContextProcessor/FallbackProcessor.php (2)
17-17: Make internal processors finalThese classes are internal implementation details; marking them final eases refactoring and enables engine optimizations.
-class FallbackProcessor implements ContextProcessorInterface +final class FallbackProcessor implements ContextProcessorInterface
19-23: Silence PHPMD UnusedFormalParameter warnings intentionallyIn canProcess the parameter is unused by design; in process the recursive callback is unused by design. Suppress these to keep CI noise down.
class FallbackProcessor implements ContextProcessorInterface { + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ public function canProcess(mixed $value): bool { // This processor can handle anything as a last resort return true; } /** * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { return \gettype($value); } }Also applies to: 25-33
src/Internal/ContextProcessor/ThrowableProcessor.php (2)
17-17: Make internal processors finalSame rationale as other processors.
-class ThrowableProcessor implements ContextProcessorInterface +final class ThrowableProcessor implements ContextProcessorInterface
24-39: Leverage the recursive processor for chained exceptionsIncluding previous exceptions makes the structured payload complete and also removes the “unused parameter” warning. Suggest processing the chain via the supplied recursor.
public function process(mixed $value, callable $recursiveProcessor): mixed { - return [ + $previous = $value->getPrevious(); + + return [ 'class' => \get_class($value), 'message' => $value->getMessage(), 'code' => $value->getCode(), 'file' => $value->getFile(), 'line' => $value->getLine(), - 'trace' => $value->getTraceAsString(), + 'trace' => $value->getTraceAsString(), + // Include previous exception chain (if any) + 'previous' => $previous !== null ? $recursiveProcessor($previous) : null, ]; }tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
44-49: PHPMD unused private fields in test fixtures are intentionalThe anonymous class’ private field is present to verify exclusion of non‑public props. Consider suppressing PHPMD for tests or configuring the ruleset to ignore tests to avoid false positives.
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (2)
122-145: Precedence control for custom processorsThis test demonstrates that custom processors added after defaults can’t override built‑ins (strings handled by BuiltInTypeProcessor). Consider supporting a “prepend” registration to enable targeted overrides.
Example API and usage:
- public function addProcessor(ContextProcessorInterface $processor): void + public function addProcessor(ContextProcessorInterface $processor): void { $this->processors[] = $processor; } + + /** + * Register a processor with higher priority (checked before existing ones). + */ + public function prependProcessor(ContextProcessorInterface $processor): void + { + \array_unshift($this->processors, $processor); + }Then in the test:
- $this->manager->addProcessor($customProcessor); + $this->manager->prependProcessor($customProcessor);And assert the transformed value accordingly.
25-38: Optional: add a test asserting numeric keys become stringsprocessContext stringifies keys; add a case like
['0' => 'a']and[0 => 'a']to ensure stable behavior and set expectations in tests.src/RpcLogger.php (1)
22-26: Constructor injection of ContextProcessorManager looks goodOptional enhancement: rename the property to $contextProcessorManager for clarity (it holds a manager, not a single processor). Not a blocker.
tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php (1)
17-32: Avoid leaking resources from the data providerThe provider opens a stream that is never closed. Wrap the resource creation in a closure and close it in the test after the assertion.
Apply this diff:
public static function builtInTypeValuesProvider(): array { return [ @@ - 'resource' => [\fopen('php://memory', 'r'), false], + // Defer resource creation to the test to ensure proper closing + 'resource' => [static fn() => \fopen('php://memory', 'r'), false], ]; } @@ - public function testCanProcessBuiltInTypes(mixed $value, bool $expected): void + public function testCanProcessBuiltInTypes(mixed $value, bool $expected): void { - $this->assertSame($expected, $this->processor->canProcess($value)); + if (\is_callable($value)) { + $value = $value(); + } + try { + $this->assertSame($expected, $this->processor->canProcess($value)); + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }tests/Unit/ContextProcessor/ResourceProcessorTest.php (3)
43-53: Use try/finally to guarantee stream closure on assertion failuresMinor hygiene to prevent leaks if an assertion throws.
public function testProcessStreamResource(): void { - $resource = \fopen('php://memory', 'r'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - - $this->assertSame('stream resource', $result); + $resource = \fopen('php://memory', 'r'); + try { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + $this->assertSame('stream resource', $result); + } finally { + \fclose($resource); + } }
55-67: Same: ensure file handle cleanup via try/finallyKeeps temp file and handle lifecycle safe even on failures.
public function testProcessFileResource(): void { $tempFile = \tempnam(\sys_get_temp_dir(), 'test'); - $resource = \fopen($tempFile, 'w'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - \unlink($tempFile); - - $this->assertSame('stream resource', $result); + $resource = \fopen($tempFile, 'w'); + try { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + $this->assertSame('stream resource', $result); + } finally { + \fclose($resource); + \unlink($tempFile); + } }
69-88: Tidy up cURL test: ensure handles are always closedHandles both CurlHandle object (PHP 8+) and resource cleanup deterministically.
public function testProcessCurlResource(): void { if (!\extension_loaded('curl')) { $this->markTestSkipped('cURL extension not available'); } - $curlHandle = \curl_init(); - - // In PHP 8+, curl_init returns CurlHandle object, not resource - // Let's test with a different resource type that's consistently a resource - $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - \curl_close($curlHandle); - - $this->assertSame('stream resource', $result); + $curlHandle = \curl_init(); + $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); + try { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + $this->assertSame('stream resource', $result); + } finally { + \fclose($resource); + if (\is_resource($curlHandle) || $curlHandle !== null) { + \curl_close($curlHandle); + } + } }src/Internal/ContextProcessor/ContextProcessorManager.php (1)
64-77: processValue() is now redundant if the guard is inlined into processContextEither:
- keep processValue() as a thin wrapper that calls the guarded closure (for tests using processValue directly), or
- deprecate processValue() in favor of the guarded closure pattern.
If tests call processValue() directly, provide a compatibility wrapper:
- public function processValue(mixed $value): mixed + public function processValue(mixed $value): mixed { - foreach ($this->processors as $processor) { - if ($processor->canProcess($value)) { - return $processor->process($value, [$this, 'processValue']); - } - } - // This should never happen due to FallbackProcessor, but just in case - return \gettype($value); + // Compatibility path without guard; consider routing via processContext for safety + foreach ($this->processors as $processor) { + if ($processor->canProcess($value)) { + return $processor->process($value, [$this, 'processValue']); + } + } + return \gettype($value); }Up to you whether to route it through the guarded path; if you do, annotate the potential performance overhead.
Would you like me to adapt tests (ContextProcessorManagerTest et al.) if you choose to route processValue through the guarded path?
tests/Unit/RpcLoggerTest.php (1)
466-470: Suppress PHPMD false-positive for intentionally unused private/protected props in object-fixture.Private/protected props are present by design to validate that ObjectProcessor only extracts public properties. PHPMD reports UnusedPrivateField here. Add an inline suppression on the private property to keep static analysis green without altering the fixture’s intent.
- private string $privateProp = 'private value'; + /** @SuppressWarnings(PHPMD.UnusedPrivateField) */ + private string $privateProp = 'private value';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
composer.json(1 hunks)src/Internal/ContextProcessor/BuiltInTypeProcessor.php(1 hunks)src/Internal/ContextProcessor/ContextProcessorInterface.php(1 hunks)src/Internal/ContextProcessor/ContextProcessorManager.php(1 hunks)src/Internal/ContextProcessor/DateTimeProcessor.php(1 hunks)src/Internal/ContextProcessor/FallbackProcessor.php(1 hunks)src/Internal/ContextProcessor/ObjectProcessor.php(1 hunks)src/Internal/ContextProcessor/ResourceProcessor.php(1 hunks)src/Internal/ContextProcessor/StringableProcessor.php(1 hunks)src/Internal/ContextProcessor/ThrowableProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ContextProcessorManagerTest.php(1 hunks)tests/Unit/ContextProcessor/DateTimeProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/FallbackProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ObjectProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ResourceProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/StringableProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ThrowableProcessorTest.php(1 hunks)tests/Unit/RpcLoggerTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Unit/ContextProcessor/DateTimeProcessorTest.php
- tests/Unit/ContextProcessor/StringableProcessorTest.php
- tests/Unit/ContextProcessor/FallbackProcessorTest.php
🧰 Additional context used
🧬 Code graph analysis (16)
src/Internal/ContextProcessor/StringableProcessor.php (3)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)
src/Internal/ContextProcessor/FallbackProcessor.php (6)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/ObjectProcessor.php (6)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/ResourceProcessor.php (1)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
src/Internal/ContextProcessor/ContextProcessorInterface.php (7)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (2)
canProcess(125-128)process(130-133)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/DateTimeProcessor.php (1)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
tests/Unit/RpcLoggerTest.php (10)
src/Internal/ContextProcessor/ContextProcessorManager.php (2)
ContextProcessorManager(15-102)addProcessor(29-32)src/RpcLogger.php (2)
log(35-59)RpcLogger(15-60)tests/Unit/RpcSpy.php (4)
getCallCount(42-45)getLastCall(37-40)RpcSpy(17-51)reset(47-50)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/ContextProcessorManager.php (9)
src/RpcLogger.php (1)
__construct(22-26)src/Internal/ContextProcessor/BuiltInTypeProcessor.php (3)
canProcess(22-25)process(32-53)BuiltInTypeProcessor(20-54)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (3)
canProcess(19-22)process(29-32)DateTimeProcessor(17-33)src/Internal/ContextProcessor/FallbackProcessor.php (3)
canProcess(19-23)process(29-32)FallbackProcessor(17-33)src/Internal/ContextProcessor/ObjectProcessor.php (3)
canProcess(19-22)process(29-50)ObjectProcessor(17-51)src/Internal/ContextProcessor/ResourceProcessor.php (3)
canProcess(18-21)process(28-31)ResourceProcessor(16-32)src/Internal/ContextProcessor/StringableProcessor.php (3)
canProcess(18-21)process(28-31)StringableProcessor(16-32)src/Internal/ContextProcessor/ThrowableProcessor.php (3)
canProcess(19-22)process(29-39)ThrowableProcessor(17-40)
src/Internal/ContextProcessor/ThrowableProcessor.php (5)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)
tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php (2)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (5)
canProcess(125-128)process(130-133)testProcessScalarValues(25-38)testProcessNestedArray(81-95)setUp(204-207)src/Internal/ContextProcessor/BuiltInTypeProcessor.php (3)
BuiltInTypeProcessor(20-54)canProcess(22-25)process(32-53)
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (2)
src/Internal/ContextProcessor/ThrowableProcessor.php (3)
ThrowableProcessor(17-40)canProcess(19-22)process(29-39)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
tests/Unit/ContextProcessor/ResourceProcessorTest.php (2)
src/Internal/ContextProcessor/ResourceProcessor.php (3)
ResourceProcessor(16-32)canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
src/Internal/ContextProcessor/BuiltInTypeProcessor.php (1)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (2)
canProcess(125-128)process(130-133)
src/RpcLogger.php (1)
src/Internal/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(15-102)__construct(20-23)processContext(42-62)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (9)
src/Internal/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(15-102)processContext(42-62)addProcessor(29-32)src/Internal/ContextProcessor/BuiltInTypeProcessor.php (2)
canProcess(22-25)process(32-53)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
tests/Unit/ContextProcessor/ObjectProcessorTest.php (2)
src/Internal/ContextProcessor/ObjectProcessor.php (3)
ObjectProcessor(17-51)canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
🪛 PHPMD (2.15.0)
src/Internal/ContextProcessor/StringableProcessor.php
28-28: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/FallbackProcessor.php
19-19: Avoid unused parameters such as '$value'. (Unused Code Rules)
(UnusedFormalParameter)
29-29: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/ResourceProcessor.php
28-28: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/DateTimeProcessor.php
29-29: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/RpcLoggerTest.php
468-468: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
553-553: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
610-610: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
677-677: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/ThrowableProcessor.php
29-29: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php
113-113: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
130-130: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ObjectProcessorTest.php
48-48: Avoid unused private fields such as '$secret'. (Unused Code Rules)
(UnusedPrivateField)
64-64: Avoid unused private fields such as '$private'. (Unused Code Rules)
(UnusedPrivateField)
🔇 Additional comments (16)
src/Internal/ContextProcessor/StringableProcessor.php (1)
18-21: Logic is correct: handles all Stringable objects via cast
canProcessand(string)$valueare precise and side-effect free. No issues.src/Internal/ContextProcessor/DateTimeProcessor.php (1)
19-22: LGTM: robust DateTime detectionCovers DateTime and DateTimeImmutable via DateTimeInterface. Clean and predictable.
src/Internal/ContextProcessor/ResourceProcessor.php (1)
18-21: Resource detection is correct
is_resourceis the right gate; behavior is deterministic even for closed resources.src/Internal/ContextProcessor/ContextProcessorInterface.php (1)
7-17: Interface shape and documentation look solidClear internal contract, succinct docs, and practical generics for static analyzers. No issues.
src/Internal/ContextProcessor/ThrowableProcessor.php (1)
31-38: Consider configurable trace verbosity for PII/sizing controlTrace/file/line often contain file paths and can be large. If this is emitted in prod logs, add a way (config flag or alternate processor) to omit or truncate these fields.
Do you want me to sketch a minimal “SafeThrowableProcessor” variant and corresponding tests?
src/RpcLogger.php (1)
45-47: Nice: context is processed via ContextProcessorManager before dispatchThis is the right place to normalize/serialize context once, keeping the switch clean.
src/Internal/ContextProcessor/BuiltInTypeProcessor.php (2)
22-25: Good coverage of built-in types with fast-path checkcanProcess correctly short-circuits for null|scalar|array, keeping the common path cheap.
34-53: Array recursion is correct; preserve keys and delegate via callbackImplementation is clean and allocates a fresh array. No change required.
tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php (1)
68-101: Nice recursive test; mirrors manager callback behaviorThe nested array test demonstrates the contract expected by processors when delegating via the recursive callback.
src/Internal/ContextProcessor/ContextProcessorManager.php (1)
83-101: Order of default processors is sensible and future-proofBuilt-in types first, specifics next, resource, generic object, then fallback. Matches expected hit rates and specificity.
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (4)
17-26: Solid coverage of Throwable variants in provider.Representative set (Exception, RuntimeException, InvalidArgumentException, Error, TypeError) makes canProcess() behavior explicit. No changes needed.
28-38: Good negative cases for canProcess().Covers common non-throwable inputs including null and arrays. Looks good.
52-69: End-to-end structure assertions are appropriate.Validates class/message/code and basic types for file/line/trace for both Exception and Error. These will catch regressions in ThrowableProcessor::process().
Also applies to: 71-82
97-101: Test fixture initialization is clear.setUp() constructs a fresh processor per test. Good.
tests/Unit/RpcLoggerTest.php (2)
359-376: Broad context coverage looks good.These tests exercise scalars, DateTimeInterface, Throwable, Stringable, nested arrays, plain objects, resources, and mixed contexts, asserting the correct RPC method selection and payload envelope type. This gives confidence that RpcLogger delegates correctly once ContextProcessorManager is involved.
Also applies to: 378-396, 397-413, 415-436, 438-461, 463-483, 485-503, 505-542
14-16: New imports align with the processor-based context pipeline.Importing ContextProcessorInterface and ContextProcessorManager here is appropriate for the added tests.
| match ($normalizedLevel) { | ||
| PsrLogLevel::EMERGENCY, | ||
| PsrLogLevel::ALERT, | ||
| PsrLogLevel::CRITICAL, | ||
| PsrLogLevel::ERROR => $this->logger->error($message, $context), | ||
| PsrLogLevel::WARNING => $this->logger->warning($message, $context), | ||
| PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $context), | ||
| 'log' => $this->logger->log((string) $message, $context), | ||
| PsrLogLevel::DEBUG => $this->logger->debug($message, $context), | ||
| PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), | ||
| PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), | ||
| PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), | ||
| 'log' => $this->logger->log((string) $message, $processedContext), | ||
| PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify: is custom level 'log' intentionally supported as an input level?
PSR-3 levels don’t include 'log'. Keeping this branch is fine if RoadRunner’s Logger explicitly accepts a 'log' level alias; otherwise consider removing it to avoid accepting invalid levels.
🌐 Web query:
Does RoadRunner\Logger\Logger accept a 'log' level (as input) distinct from PSR-3 levels, or is 'log' only a generic method name? If not supported as a level, confirm recommended handling for unknown levels.
💡 Result:
RoadRunner\Logger\Logger does not accept a 'log' level as a distinct log level; 'log' is only the generic method name defined by PSR-3 for submitting logs at any severity. Only the standard PSR-3 levels (emergency, alert, critical, error, warning, notice, info, debug) are accepted; custom or unknown levels, such as a literal 'log' level, are not supported[6][4][10].
Supporting details:
- PSR-3 Compliance: RoadRunner’s logging is designed to be PSR-3 compliant[1][5]. This means it follows the Psr\Log\LoggerInterface which defines the
log($level, $message, array $context = [])method. The method namedlogis generic and allows you to specify any compliant log level as the first argument, but the levels themselves must conform to the eight PSR-3 levels[6]. - Allowed Levels: Official RoadRunner documentation and its logger plugin describe supported levels for configuration and filtering as debug, info, warn, error, panic[4][10]. However, for PHP userland and the app-logger RPC plugin, only the standard PSR-3 levels are valid as input.
- Purpose of
logMethod: Thelogmethod’s purpose is to allow for passing the level dynamically—it is not a substitute or equivalent for a 'log' level[6]. - Handling Unknown Levels: The PSR-3 specification and common implementations (as seen in Monolog and others) require that if an unknown or unsupported level is provided, an InvalidArgumentException must be thrown[3][6]. Your implementation should therefore either:
- Map only valid PSR-3 levels to the logger.
- Throw an exception or otherwise reject unknown (non-PSR-3) levels.
Summary of best practice:
Use only the officially recognized PSR-3 log levels (emergency, alert, critical, error, warning, notice, info, debug). If an unknown level, such as 'log', is provided, reject it with a suitable exception (e.g., InvalidArgumentException) as per PSR-3 guidelines[3][6].
Additional context:
- RoadRunner’s internal logging and config also allow for levels like 'panic' in addition to PSR-3, but these are for internal server-side config/transport and not exposed as input levels to the application-side logger[4][10].
- If you need custom levels, you’ll need to map them to one of the supported levels before passing to RoadRunner\Logger\Logger, or log them as context data within a permitted level.
Citations:
- 1: https://packagist.org/packages/roadrunner/psr-logger
- 2: https://betterstack.com/community/guides/scaling-php/php-roadrunner/
- 3: PSR-3 compliant logger simplesamlphp/simplesamlphp#371
- 4: https://docs.roadrunner.dev/docs/logging-and-observability/logger
- 5: https://packagist.org/packages/eplightning/roadrunner-logger
- 6: https://www.sitepoint.com/logging-with-psr-3-to-improve-reusability/
- 7: https://sys-bio.github.io/roadrunner/docs-build/PythonAPIReference/cls_Logger.html
- 8: https://docs.roadrunner.dev/docs/general/config
- 9: https://snap.berkeley.edu/project/11166188
- 10: https://docs.roadrunner.dev/docs/logging-and-observability/applogger
Remove unsupported ‘log’ level and enforce PSR-3 compliance
The ‘log’ branch currently treats “log” as if it were a valid PSR-3 level, but RoadRunner’s PHP logger only recognizes the eight standard PSR-3 levels (emergency, alert, critical, error, warning, notice, info, debug). The log() method itself is a generic entry point, not a distinct level. Accepting “log” silently risks masking invalid input.
• Remove the 'log' => $this->logger->log((string) $message, $processedContext), arm from the match statement.
• Add a default arm that throws an InvalidArgumentException for any unrecognized level, e.g.:
default => throw new \Psr\Log\InvalidArgumentException(
sprintf('Unsupported log level "%s". Valid levels: emergency, alert, critical, error, warning, notice, info, debug.', $normalizedLevel)
),• Update or add tests to assert that providing an invalid level (including “log”) triggers the exception.
This change ensures that only valid PSR-3 levels are accepted and that any unknown level is explicitly rejected, in line with PSR-3 guidelines.
🤖 Prompt for AI Agents
In src/RpcLogger.php around lines 48 to 56, remove the 'log' match arm (which
treats "log" as a level) and replace the match with one that only handles the
eight PSR-3 levels; add a default arm that throws a
\Psr\Log\InvalidArgumentException with a message like 'Unsupported log level
"%s". Valid levels: emergency, alert, critical, error, warning, notice, info,
debug.' (using sprintf to include the invalid level); and add/update tests to
assert that passing an invalid level (including "log") causes this exception to
be thrown.
|
@roxblnfk do you have any updates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Internal/ObjectProcessor.php (1)
8-8: Fix doc typo for clarity.
The summary line currently says “arra” instead of “array”. Please correct the typo to keep the PHPDoc accurate.- * Converts an object into a scalar or an arra for serializable logger context. + * Converts an object into a scalar or an array for serializable logger context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Internal/ContextProcessor/DateTimeProcessor.php(1 hunks)src/Internal/ContextProcessor/FallbackProcessor.php(1 hunks)src/Internal/ContextProcessor/StringableProcessor.php(1 hunks)src/Internal/ContextProcessor/ThrowableProcessor.php(1 hunks)src/Internal/DefaultProcessor.php(1 hunks)src/Internal/ObjectProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/Internal/ContextProcessor/ThrowableProcessor.php (4)
src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Internal/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
src/Internal/ContextProcessor/StringableProcessor.php (4)
src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)src/Internal/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
src/RpcLogger.php (2)
src/Internal/DefaultProcessor.php (2)
DefaultProcessor(12-60)createDefault(17-26)tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
__construct(107-111)
src/Internal/ObjectProcessor.php (4)
src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)
src/Internal/ContextProcessor/FallbackProcessor.php (4)
src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)src/Internal/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
src/Internal/ContextProcessor/DateTimeProcessor.php (2)
src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Internal/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
src/Internal/DefaultProcessor.php (5)
src/Internal/ContextProcessor/DateTimeProcessor.php (3)
DateTimeProcessor(19-30)canProcess(21-24)process(26-29)src/Internal/ContextProcessor/FallbackProcessor.php (3)
FallbackProcessor(16-36)canProcess(18-21)process(23-35)src/Internal/ContextProcessor/StringableProcessor.php (3)
StringableProcessor(16-27)canProcess(18-21)process(23-26)src/Internal/ContextProcessor/ThrowableProcessor.php (3)
ThrowableProcessor(17-35)canProcess(19-22)process(24-34)src/Internal/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
🪛 PHPMD (2.15.0)
src/Internal/ContextProcessor/ThrowableProcessor.php
24-24: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Internal/ContextProcessor/StringableProcessor.php
23-23: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Internal/ContextProcessor/FallbackProcessor.php
18-18: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
src/Internal/ContextProcessor/DateTimeProcessor.php
26-26: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
🪛 GitHub Actions: phpunit
src/Internal/ContextProcessor/ThrowableProcessor.php
[error] 1-1: Class 'RoadRunner\PsrLogger\Internal\ContextProcessor\ThrowableProcessor' not found.
src/Internal/ContextProcessor/StringableProcessor.php
[error] 1-1: Class 'RoadRunner\PsrLogger\Internal\ContextProcessor\StringableProcessor' not found.
src/RpcLogger.php
[error] 27-27: RpcLogger::constructor: Default processor creation failed due to type error/return issue.
src/Internal/ContextProcessor/FallbackProcessor.php
[error] 18-18: FallbackProcessor::canProcess() received non-object: expected object parameter
src/Internal/ContextProcessor/DateTimeProcessor.php
[error] 21-21: DateTimeProcessor.canProcess() argument type mismatch: expected object, got string
src/Internal/DefaultProcessor.php
[error] 26-26: DefaultProcessor::createDefault() return type mismatch: should return DefaultProcessor instance, none returned
[error] 27-27: RpcLogger: Default processor creation failed due to return type mismatch
🪛 GitHub Actions: static analysis
src/RpcLogger.php
[error] 27-27: InvalidPropertyAssignmentValue: $this->contextProcessor with declared type 'RoadRunner\PsrLogger\Internal\ObjectProcessor' cannot be assigned type 'RoadRunner\PsrLogger\Internal\DefaultProcessor|callable'
[error] 48-48: MixedAssignment: Unable to determine the type that $processedContext is being assigned to
[error] 48-48: UndefinedInterfaceMethod: Method RoadRunner\PsrLogger\Internal\ObjectProcessor::processContext does not exist
[error] 54-54: MixedArgument: Argument 2 of RoadRunner\Logger\Logger::error cannot be mixed, expecting array<string, mixed>
src/Internal/ContextProcessor/FallbackProcessor.php
[error] 23-23: InvalidReturnType: The declared return type 'string' for RoadRunner\PsrLogger\Internal\ContextProcessor\FallbackProcessor::process is incorrect, got 'array<string, mixed>'
[error] 26-26: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 31-31: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 34-34: InvalidReturnStatement: The inferred type 'array<string, mixed>' does not match the declared return type 'string' for RoadRunner\PsrLogger\Internal\ContextProcessor\FallbackProcessor::process
src/Internal/DefaultProcessor.php
[error] 17-17: InvalidReturnType: Not all code paths of RoadRunner\PsrLogger\Internal\DefaultProcessor::createDefault end in a return statement, return type RoadRunner\PsrLogger\Internal\DefaultProcessor expected
[error] 45-45: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 46-46: MixedAssignment: Unable to determine the type that $v is being assigned to
🔇 Additional comments (1)
src/RpcLogger.php (1)
57-59: Drop the unsupported'log'pseudo-level.
“log” isn’t a PSR‑3 severity. Keeping this branch masks invalid input levels instead of throwing theInvalidArgumentExceptionyou already provide in the default arm. Please remove the'log'case so unknown levels are rejected.- PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), - 'log' => $this->logger->log((string) $message, $processedContext), + PsrLogLevel::NOTICE, + PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext),
| $result = ['@class' => $value::class] + \get_object_vars($value); | ||
| foreach ($result as $k => &$v) { | ||
| if ($v === $value) { | ||
| unset($result[$k]); | ||
| } | ||
|
|
||
| $v = $processor($v); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skip processing self-references after unsetting them.
Line 27 tries to drop self-referential properties, but because the loop lacks a continue, the same reference is immediately re-processed on Line 31, which reintroduces the entry and recurses indefinitely. Add a continue; right after the unset() so self-references stay removed and the recursion guard actually works.
Apply this diff:
foreach ($result as $k => &$v) {
if ($v === $value) {
unset($result[$k]);
+ continue;
}
$v = $processor($v);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $result = ['@class' => $value::class] + \get_object_vars($value); | |
| foreach ($result as $k => &$v) { | |
| if ($v === $value) { | |
| unset($result[$k]); | |
| } | |
| $v = $processor($v); | |
| } | |
| $result = ['@class' => $value::class] + \get_object_vars($value); | |
| foreach ($result as $k => &$v) { | |
| if ($v === $value) { | |
| unset($result[$k]); | |
| continue; | |
| } | |
| $v = $processor($v); | |
| } |
🧰 Tools
🪛 GitHub Actions: static analysis
[error] 26-26: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 31-31: MixedAssignment: Unable to determine the type that $v is being assigned to
🤖 Prompt for AI Agents
In src/Internal/ContextProcessor/FallbackProcessor.php around lines 25 to 32,
the foreach unsets self-referential properties but then continues to process the
same referenced variable, which re-adds it and causes infinite recursion; after
unsetting the entry add a `continue;` so the loop skips further processing of
that item and the self-reference remains removed, preserving the recursion
guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (13)
src/Internal/ContextProcessor/FallbackProcessor.php (2)
23-25: Add precise docblocks to fix mixed assignment warnings.Annotate the shape for Psalm/PhpStan.
- $result = ['@class' => $value::class] + \get_object_vars($value); + /** @var array<string, mixed> $result */ + $result = ['@class' => $value::class] + \get_object_vars($value);Optionally, add
@psalm-return array<string, mixed>to the method.
25-25: Consider standardizing the key name with ThrowableProcessor.This uses '@Class' while ThrowableProcessor uses 'class'. Pick one for consistency (prefer 'class' unless '@' is intentional for namespacing).
src/RpcLogger.php (2)
20-26: Type the callable and construct the closure explicitly for analyzers.Help static analysis by documenting the callable signature.
- private readonly \Closure $objectProcessor; + /** @var \Closure(mixed): mixed */ + private readonly \Closure $objectProcessor; @@ - $this->objectProcessor = ($processor ?? DefaultProcessor::createDefault())(...); + /** @var \Closure(mixed): mixed $callable */ + $callable = ($processor ?? DefaultProcessor::createDefault())(...); + $this->objectProcessor = $callable;
35-56: Cast message once; remove per-branch casts.Guarantee string messages under strict types and reduce duplication.
public function log($level, \Stringable|string $message, array $context = []): void { + $message = (string) $message; @@ - PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), - PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), - PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), - 'log' => $this->logger->log((string) $message, $processedContext), - PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), + PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), + PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), + PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info($message, $processedContext), + 'log' => $this->logger->log($message, $processedContext), + PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext),tests/Unit/RpcLoggerTest.php (9)
360-377: These scalar-context tests don’t validate processing outcome.They only assert call count and payload type. Add an assertion that the scalar values survive unchanged to the payload’s context map (e.g., via a test double that exposes context), otherwise the test can’t catch regressions.
379-396: DateTime context: assert formatted value if possible.If you can access the payload’s context, assert ATOM formatting for both DateTime and DateTimeImmutable. Otherwise, this test won’t detect formatter regressions.
398-414: Throwable context: assert shape (class/message/code/trace) if accessible.Strengthen by asserting the serialized keys to detect breaking changes in
ThrowableProcessor.
416-437: Stringable context: verify casted string landed in context.If payload allows, assert
'user' => 'Custom stringable object'. Helps validateStringableProcessor.
439-462: Nested arrays: assert deep preservation.Consider asserting a deep key (e.g.,
['level1']['level2']['level3']['number'] === 123) in the processed context.
464-484: Object context: clarify visibility expectations.Fallback uses
get_object_varsand will include only accessible/public props. Add an assertion for presence ofpublicPropand absence of private/protected if you can read the context.
486-504: Resource context: good cleanup; consider asserting rendered type.You close the resource—good. If accessible, assert
'file_handle' => 'stream resource'.
506-543: Mixed complex context: add at least one ground-truth assertion.E.g., ensure
InvalidArgumentExceptionserialized andDateTimeformatted. Otherwise, this test duplicates earlier coverage without validating transformation.
583-635: Same issue for URL/CC processors; also missing assertions of transformed context.These processors target strings and won’t be reached. Strengthen tests by asserting URL parts and masked card if/when the pipeline supports scalar processors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Internal/ContextProcessor/FallbackProcessor.php(1 hunks)src/Internal/DefaultProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/RpcLoggerTest.php(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Internal/DefaultProcessor.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Unit/RpcLoggerTest.php (7)
src/Internal/DefaultProcessor.php (3)
DefaultProcessor(12-61)createDefault(17-27)withObjectProcessors(32-37)src/RpcLogger.php (2)
log(35-59)RpcLogger(15-60)tests/Unit/RpcSpy.php (4)
getCallCount(42-45)getLastCall(37-40)RpcSpy(17-51)reset(47-50)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)
src/RpcLogger.php (1)
src/Internal/DefaultProcessor.php (2)
DefaultProcessor(12-61)createDefault(17-27)
src/Internal/ContextProcessor/FallbackProcessor.php (4)
src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)src/Internal/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
🪛 GitHub Actions: phpunit
src/Internal/ContextProcessor/FallbackProcessor.php
[error] 18-18: FallbackProcessor::canProcess() TypeError: first argument must be of type object; scalar given (in test FallbackProcessorTest).
🪛 GitHub Actions: static analysis
src/RpcLogger.php
[error] 46-46: MixedAssignment: Unable to determine the type that $processedContext is being assigned to
[error] 52-52: MixedArgument: Argument 2 of RoadRunner\Logger\Logger::error cannot be mixed, expecting array<string, mixed>
src/Internal/ContextProcessor/FallbackProcessor.php
[error] 26-26: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 31-31: MixedAssignment: Unable to determine the type that $v is being assigned to
🪛 PHPMD (2.15.0)
tests/Unit/RpcLoggerTest.php
469-469: Avoid unused private fields such as '$privateProp'. (undefined)
(UnusedPrivateField)
554-554: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
610-610: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
677-677: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Internal/ContextProcessor/FallbackProcessor.php
18-18: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (3)
tests/Unit/RpcLoggerTest.php (3)
62-62: LGTM: payload type assertions.Checking for
LogEntrypayloads at these call sites is fine. Consider adding one test that also inspects the serialized context if feasible.Also applies to: 75-75, 89-89, 102-102, 145-145, 157-157
637-654: Default processor creation test: LGTM.Confirms no-constructor-arg fallback path.
701-713: Test harness hygiene: good use of tearDown to reset spy.Keeps tests isolated.
| foreach ($result as $k => &$v) { | ||
| if ($v === $value) { | ||
| unset($result[$k]); | ||
| } | ||
|
|
||
| $v = $processor($v); | ||
| } | ||
|
|
||
| return $result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Prevent re-processing after unsetting self-references; avoid by-ref pitfalls.
After unset($result[$k]), the loop immediately reuses $v, which can reintroduce the value and risks runaway recursion. Also, iterating by reference leaks $v beyond the loop. Refactor to key-based writes (no &$v), and skip once unset.
Apply:
- foreach ($result as $k => &$v) {
- if ($v === $value) {
- unset($result[$k]);
- }
-
- $v = $processor($v);
- }
+ foreach (\array_keys($result) as $k) {
+ $v = $result[$k];
+ if ($v === $value) {
+ unset($result[$k]);
+ continue;
+ }
+ $result[$k] = $processor($v);
+ }This removes the reference, skips removed entries, and satisfies static analyzers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ($result as $k => &$v) { | |
| if ($v === $value) { | |
| unset($result[$k]); | |
| } | |
| $v = $processor($v); | |
| } | |
| return $result; | |
| foreach (\array_keys($result) as $k) { | |
| $v = $result[$k]; | |
| if ($v === $value) { | |
| unset($result[$k]); | |
| continue; | |
| } | |
| $result[$k] = $processor($v); | |
| } | |
| return $result; |
🧰 Tools
🪛 GitHub Actions: static analysis
[error] 26-26: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 31-31: MixedAssignment: Unable to determine the type that $v is being assigned to
🤖 Prompt for AI Agents
In src/Internal/ContextProcessor/FallbackProcessor.php around lines 26 to 34,
the foreach uses a by-reference loop and reuses $v after an unset which can
reintroduce values and leak the reference; change to a key-based loop (foreach
($result as $k => $v)) so you can unset($result[$k]) and immediately continue
when $v === $value, and then assign the processed result back to $result[$k]
(e.g. $result[$k] = $processor($v)); this removes the reference usage, prevents
re-processing of removed entries, and avoids leaking $v beyond the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Context/ObjectProcessor/FallbackProcessorTest.php (1)
87-90: Fix incorrect processor instantiation.Line 89 instantiates
DefaultProcessorinstead ofFallbackProcessor, which is the class under test.Apply this diff:
protected function setUp(): void { - $this->processor = new DefaultProcessor(); + $this->processor = new FallbackProcessor(); }
♻️ Duplicate comments (6)
tests/Unit/Context/ObjectProcessor/ThrowableProcessorTest.php (1)
79-90: Fix brittle assertion for anonymous exception class naming.The assertion
str_contains($result['class'], 'Exception@anonymous')is fragile. PHP'sget_class()for anonymous classes returns a name likeclass@anonymous\x00...(notException@anonymous), so this assertion may fail.Apply this fix:
- $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous')); + $this->assertStringContainsString('@anonymous', $result['class']);src/Context/ObjectProcessor/FallbackProcessor.php (1)
26-32: Fix recursion + mixed assignments: avoid by-ref loop and skip after unset.Current by-ref iteration reprocesses removed self-references and leaks the reference; also triggers Psalm MixedAssignment. Refactor as key-based loop and continue after unset.
- $result = ['@class' => $value::class] + \get_object_vars($value); - foreach ($result as $k => &$v) { - if ($v === $value) { - unset($result[$k]); - } - - $v = $processor($v); - } + /** @var array<string, mixed> $result */ + $result = ['@class' => $value::class] + \get_object_vars($value); + foreach (\array_keys($result) as $k) { + $v = $result[$k]; + if ($v === $value) { + unset($result[$k]); + continue; + } + $result[$k] = $processor($v); + }src/RpcLogger.php (1)
52-56: Cast message for all branches; drop unsupported 'log' level.Avoid TypeError with \Stringable and keep PSR-3 levels only.
- PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), - PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), - PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), - 'log' => $this->logger->log((string) $message, $processedContext), - PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), + PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), + PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), + PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info($message, $processedContext), + PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext),tests/Unit/RpcLoggerTest.php (3)
545-581: Custom email processor will never execute.The email processor checks for string values (line 549), but
DefaultProcessoronly invokes object processors (lines 51-57 in src/Context/DefaultProcessor.php). Strings are scalars, so this processor is never called and email masking doesn't happen. The test only verifies an RPC call occurred, not that masking worked.To fix, either:
- Wrap email strings in value objects (e.g.,
Stringable) so the object processor pipeline can handle them, or- Extend the design to support scalar processors alongside object processors.
For option 1, add to the test:
// Before line 563 class Email implements \Stringable { public function __construct(private string $email) {} public function __toString(): string { return $this->email; } public function getValue(): string { return $this->email; } } // Update the email processor (line 549-551) public function canProcess(object $value): bool { return $value instanceof Email; } // Update process method (line 554-559) public function process(object $value, callable $processor): mixed { $email = $value->getValue(); $parts = \explode('@', $email); return \substr($parts[0], 0, 2) . '***@' . $parts[1]; } // Update context (lines 568-573) $context = [ 'user_email' => new Email('[email protected]'), 'admin_email' => new Email('[email protected]'), 'regular_string' => 'not an email', 'user_id' => 123, ];Then assert the processed result actually contains masked emails.
583-635: Custom URL and credit card processors will never execute.Both processors target strings (lines 589, 607), but the pipeline only routes objects to processors. These processors are never invoked, so URL parsing and CC masking don't occur. The test only checks that an RPC call happened.
Apply the same value-object wrapping approach as suggested for the email processor test: create
URLandCreditCardclasses implementingStringable, update the processors to checkinstanceof, and update the context to use these wrapper objects.
656-699: Processor ordering test doesn't exercise ordering.Both processors target integers (lines 662, 674), which are scalars. The object-processor pipeline never routes scalars to processors, so neither processor runs and ordering cannot be validated.
To test ordering, both processors must target the same object type. For example:
class Counter { public function __construct(public int $value) {} } $firstProcessor = new class implements ObjectProcessor { public function canProcess(object $value): bool { return $value instanceof Counter; } public function process(object $value, callable $processor): mixed { return 'first:' . $value->value; } }; // Similar for secondProcessor... $context = ['number' => new Counter(42)];Then assert that the processed context reflects the first processor's transformation.
🧹 Nitpick comments (4)
tests/Unit/Context/ObjectProcessor/StringableProcessorTest.php (1)
17-46: Clarify data provider and test naming.The
nonStringableProviderincludes aStringablecase (returningtrue), andtestCannotProcessNonStringableexpects bothtrueandfalse. This is confusing: the names imply "cannot process non-Stringable" but the implementation tests both positive and negative cases.Consider renaming for clarity:
- public static function nonStringableProvider(): array + public static function canProcessProvider(): array { return [ - [new \stdClass(), false], + 'non-stringable' => [new \stdClass(), false], - [new class implements \Stringable { + 'stringable' => [new class implements \Stringable { public function __toString(): string { return 'I am string'; } }, true], ]; } - #[DataProvider('nonStringableProvider')] - public function testCannotProcessNonStringable(mixed $value, $expected): void + #[DataProvider('canProcessProvider')] + public function testCanProcess(mixed $value, bool $expected): void { $this->assertSame($expected, $this->processor->canProcess($value)); }src/Context/ObjectProcessor/FallbackProcessor.php (2)
23-25: Add callable/return annotations for Psalm.Document the processor callable and the returned array shape to quiet MixedAssignment and improve IDE hints.
- public function process(object $value, callable $processor): array + /** + * @param callable(mixed): mixed $processor + * @return array<string, mixed> + */ + public function process(object $value, callable $processor): array
18-21: Suppress PHPMD unused parameter in canProcess.Parameter is mandated by the interface. Suppress the warning instead of renaming.
- public function canProcess(object $value): bool + /** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ + public function canProcess(object $value): boolsrc/Context/ObjectProcessor/StringableProcessor.php (1)
23-26: LGTM; optionally silence unused$processor.Implementation is correct. If tools complain, add a suppression or a no-op
unset($processor);.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Context/DefaultProcessor.php(1 hunks)src/Context/ObjectProcessor.php(1 hunks)src/Context/ObjectProcessor/DateTimeProcessor.php(1 hunks)src/Context/ObjectProcessor/FallbackProcessor.php(1 hunks)src/Context/ObjectProcessor/StringableProcessor.php(1 hunks)src/Context/ObjectProcessor/ThrowableProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/Context/DefaultProcessorTest.php(1 hunks)tests/Unit/Context/ObjectProcessor/DateTimeProcessorTest.php(1 hunks)tests/Unit/Context/ObjectProcessor/FallbackProcessorTest.php(1 hunks)tests/Unit/Context/ObjectProcessor/StringableProcessorTest.php(1 hunks)tests/Unit/Context/ObjectProcessor/ThrowableProcessorTest.php(1 hunks)tests/Unit/RpcLoggerTest.php(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
src/Context/ObjectProcessor/ThrowableProcessor.php (4)
src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)src/Context/ObjectProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Context/ObjectProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Context/ObjectProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)
src/Context/ObjectProcessor/StringableProcessor.php (4)
src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)src/Context/ObjectProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Context/ObjectProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)
tests/Unit/Context/ObjectProcessor/FallbackProcessorTest.php (2)
src/Context/ObjectProcessor/FallbackProcessor.php (3)
FallbackProcessor(16-36)process(23-35)canProcess(18-21)src/Context/ObjectProcessor.php (2)
process(25-25)canProcess(19-19)
src/Context/ObjectProcessor/FallbackProcessor.php (4)
src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)src/Context/ObjectProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Context/ObjectProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)
src/Context/ObjectProcessor.php (5)
src/Context/ObjectProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Context/ObjectProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Context/ObjectProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)tests/Unit/RpcLoggerTest.php (10)
canProcess(549-552)canProcess(587-590)canProcess(605-608)canProcess(660-663)canProcess(672-675)process(554-559)process(592-600)process(610-613)process(665-668)process(677-680)
src/RpcLogger.php (1)
src/Context/DefaultProcessor.php (2)
DefaultProcessor(12-61)createDefault(17-27)
tests/Unit/Context/ObjectProcessor/StringableProcessorTest.php (2)
src/Context/ObjectProcessor/StringableProcessor.php (3)
StringableProcessor(16-27)canProcess(18-21)process(23-26)src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
src/Context/ObjectProcessor/DateTimeProcessor.php (4)
src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)src/Context/ObjectProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Context/ObjectProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)
tests/Unit/Context/ObjectProcessor/DateTimeProcessorTest.php (2)
src/Context/ObjectProcessor/DateTimeProcessor.php (3)
DateTimeProcessor(19-30)canProcess(21-24)process(26-29)src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
tests/Unit/RpcLoggerTest.php (8)
src/Context/DefaultProcessor.php (3)
DefaultProcessor(12-61)createDefault(17-27)withObjectProcessors(32-37)src/RpcLogger.php (2)
log(35-59)RpcLogger(15-60)tests/Unit/RpcSpy.php (4)
getCallCount(42-45)getLastCall(37-40)RpcSpy(17-51)reset(47-50)src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)src/Context/ObjectProcessor/DateTimeProcessor.php (2)
canProcess(21-24)process(26-29)src/Context/ObjectProcessor/FallbackProcessor.php (2)
canProcess(18-21)process(23-35)src/Context/ObjectProcessor/StringableProcessor.php (2)
canProcess(18-21)process(23-26)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(24-34)
tests/Unit/Context/DefaultProcessorTest.php (1)
src/Context/DefaultProcessor.php (1)
DefaultProcessor(12-61)
src/Context/DefaultProcessor.php (5)
src/Context/ObjectProcessor/DateTimeProcessor.php (3)
DateTimeProcessor(19-30)canProcess(21-24)process(26-29)src/Context/ObjectProcessor/FallbackProcessor.php (3)
FallbackProcessor(16-36)canProcess(18-21)process(23-35)src/Context/ObjectProcessor/StringableProcessor.php (3)
StringableProcessor(16-27)canProcess(18-21)process(23-26)src/Context/ObjectProcessor/ThrowableProcessor.php (3)
ThrowableProcessor(17-35)canProcess(19-22)process(24-34)src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
tests/Unit/Context/ObjectProcessor/ThrowableProcessorTest.php (2)
src/Context/ObjectProcessor/ThrowableProcessor.php (3)
ThrowableProcessor(17-35)canProcess(19-22)process(24-34)src/Context/ObjectProcessor.php (2)
canProcess(19-19)process(25-25)
🪛 GitHub Actions: static analysis
src/Context/ObjectProcessor/FallbackProcessor.php
[error] 26-26: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 31-31: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
src/RpcLogger.php
[error] 46-46: Psalm: MixedAssignment: Unable to determine the type that $processedContext is being assigned to
[error] 52-52: Psalm: MixedArgument: Argument 2 of RoadRunner\Logger\Logger::error cannot be mixed, expecting array<string, mixed>
src/Context/DefaultProcessor.php
[error] 46-46: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 47-47: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
🪛 PHPMD (2.15.0)
src/Context/ObjectProcessor/ThrowableProcessor.php
24-24: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Context/ObjectProcessor/StringableProcessor.php
23-23: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Context/ObjectProcessor/FallbackProcessor.php
18-18: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
src/Context/ObjectProcessor/DateTimeProcessor.php
26-26: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
tests/Unit/RpcLoggerTest.php
469-469: Avoid unused private fields such as '$privateProp'. (undefined)
(UnusedPrivateField)
554-554: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
610-610: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
677-677: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
tests/Unit/Context/ObjectProcessor/DateTimeProcessorTest.php (1)
1-57: LGTM! Comprehensive test coverage for DateTimeProcessor.The test suite thoroughly validates both
canProcessandprocessbehavior forDateTimeandDateTimeImmutableinstances, including edge cases for non-DateTime objects. The ATOM format assertions are precise and correct.src/Context/ObjectProcessor/ThrowableProcessor.php (1)
24-34: LGTM! Correct implementation of Throwable processing.The unused
$processorparameter is required by theObjectProcessorinterface contract. SinceThrowableobjects are flattened to primitive values (no nested objects requiring recursive processing), not using the parameter here is correct.tests/Unit/Context/ObjectProcessor/StringableProcessorTest.php (1)
48-93: LGTM! Thorough test coverage for process() behavior.The tests validate string conversion for various scenarios: basic conversion, complex internal logic (uppercase transformation), and edge cases (empty strings).
src/Context/ObjectProcessor.php (1)
16-25: LGTM! Clear interface contract for object processors.The interface defines a clean contract with explicit type hints and documentation for the recursive processor callable.
tests/Unit/Context/ObjectProcessor/ThrowableProcessorTest.php (1)
17-64: LGTM! Comprehensive coverage for Throwable processing.The data providers and test methods thoroughly validate
canProcessfor variousThrowabletypes and verify that theprocessmethod correctly extracts all exception/error metadata (class, message, code, file, line, trace).src/RpcLogger.php (1)
20-26: Good: closure-based processor wiring is clean and type-safe.Using a stored Closure from a first-class callable is a solid improvement.
src/Context/ObjectProcessor/DateTimeProcessor.php (1)
19-29: LGTM!The processor correctly identifies
DateTimeInterfaceinstances and formats them to ISO 8601 (ATOM) strings for structured logging. The unused$processorparameter is required by theObjectProcessorinterface contract.src/Context/DefaultProcessor.php (1)
46-47: Psalm MixedAssignment warnings are false positives.The pipeline failure on lines 46-47 flags mixed-type assignments, which are expected and correct for a generic context processor that handles arbitrary array values. These can be suppressed with
@psalm-suppress MixedAssignmentif desired.tests/Unit/RpcLoggerTest.php (2)
469-469: Private field is intentionally unused.The PHPMD warning on line 469 is a false positive. The
$privatePropfield exists to test thatFallbackProcessorcorrectly serializes objects with private properties usingget_object_vars().
708-712: LGTM!The tearDown method properly resets the RPC spy state between tests, ensuring test isolation.
| if (\is_array($value)) { | ||
| foreach ($value as &$v) { | ||
| $v = $this($v); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return after array processing.
Array processing modifies $value by reference but doesn't explicitly return. Execution falls through to line 51 (object check fails) and eventually returns on line 59. While this works, it's confusing and wastes cycles checking if an array is an object.
Apply this diff:
if (\is_array($value)) {
foreach ($value as &$v) {
$v = $this($v);
}
+ return $value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (\is_array($value)) { | |
| foreach ($value as &$v) { | |
| $v = $this($v); | |
| } | |
| } | |
| if (\is_array($value)) { | |
| foreach ($value as &$v) { | |
| $v = $this($v); | |
| } | |
| return $value; | |
| } |
🧰 Tools
🪛 GitHub Actions: static analysis
[error] 46-46: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 47-47: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
🤖 Prompt for AI Agents
In src/Context/DefaultProcessor.php around lines 45 to 49, the code processes
arrays by reference but does not explicitly return after transforming each
element, causing execution to continue and perform an unnecessary object check;
add an immediate return of the processed $value right after the foreach block
(i.e., return $value;) so the method exits once array handling is complete.
| // Process context data for structured logging using the processor manager | ||
| $processedContext = ($this->objectProcessor)($context); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normalize context keys to strings and cast message once.
Fix Psalm MixedAssignment/MixedArgument and ensure AppLogger gets array<string,mixed>.
- // Process context data for structured logging using the processor manager
- $processedContext = ($this->objectProcessor)($context);
+ // Process context data for structured logging
+ /** @var array<array-key, mixed> $tmpContext */
+ $tmpContext = ($this->objectProcessor)($context);
+ /** @var array<string, mixed> $processedContext */
+ $processedContext = [];
+ foreach ($tmpContext as $k => $v) {
+ $processedContext[(string) $k] = $v;
+ }
+ $message = (string) $message;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Process context data for structured logging using the processor manager | |
| $processedContext = ($this->objectProcessor)($context); | |
| // Process context data for structured logging | |
| /** @var array<array-key, mixed> $tmpContext */ | |
| $tmpContext = ($this->objectProcessor)($context); | |
| /** @var array<string, mixed> $processedContext */ | |
| $processedContext = []; | |
| foreach ($tmpContext as $k => $v) { | |
| $processedContext[(string) $k] = $v; | |
| } | |
| $message = (string) $message; |
🧰 Tools
🪛 GitHub Actions: static analysis
[error] 46-46: Psalm: MixedAssignment: Unable to determine the type that $processedContext is being assigned to
🤖 Prompt for AI Agents
In src/RpcLogger.php around lines 45-47, the processed context currently may
have non-string keys and mixed types which triggers Psalm
MixedAssignment/MixedArgument; cast the incoming $message to string once
up-front (e.g. $message = (string)$message) before any logging, then after
running the object processor normalize the context keys to strings and ensure
the value is an array<string,mixed> (use array_combine(array_map('strval',
array_keys($processedContext)), array_values($processedContext)) or equivalent
and fall back to an empty array on failure), and add a local docblock/type
assertion like /** @var array<string,mixed> $processedContext */ before passing
it to AppLogger so Psalm and the logger receive a proper array<string,mixed>.
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\TestCase; | ||
| use RoadRunner\PsrLogger\Context\DefaultProcessor; | ||
| use RoadRunner\PsrLogger\Context\ObjectProcessor\BuiltInTypeProcessor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import.
BuiltInTypeProcessor is imported but never used and doesn't exist in the codebase.
Apply this diff:
use RoadRunner\PsrLogger\Context\DefaultProcessor;
-use RoadRunner\PsrLogger\Context\ObjectProcessor\BuiltInTypeProcessor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use RoadRunner\PsrLogger\Context\ObjectProcessor\BuiltInTypeProcessor; | |
| use RoadRunner\PsrLogger\Context\DefaultProcessor; |
🤖 Prompt for AI Agents
In tests/Unit/Context/DefaultProcessorTest.php around line 11, the import "use
RoadRunner\PsrLogger\Context\ObjectProcessor\BuiltInTypeProcessor;" is unused
and refers to a non-existent class; remove this import line from the file so
only required imports remain, then run tests to ensure no other references to
BuiltInTypeProcessor exist.
| public function testProcessNull(): void | ||
| { | ||
| $recursiveProcessor = static fn($v) => $v; | ||
| $result = ($this->processor)(null, $recursiveProcessor); | ||
| $this->assertNull($result); | ||
| } | ||
|
|
||
| public function testProcessScalarValues(): void | ||
| { | ||
| $values = ['test string', 42, 3.14, true, false]; | ||
| $recursiveProcessor = static fn($v) => $v; | ||
|
|
||
| foreach ($values as $value) { | ||
| $result = ($this->processor)($value, $recursiveProcessor); | ||
| $this->assertSame($value, $result); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect method signatures in tests.
DefaultProcessor::__invoke() accepts only one parameter (mixed $value), but lines 43 and 53 pass a second $recursiveProcessor parameter. These tests will fail.
Apply this diff:
public function testProcessNull(): void
{
- $recursiveProcessor = static fn($v) => $v;
- $result = ($this->processor)(null, $recursiveProcessor);
+ $result = ($this->processor)(null);
$this->assertNull($result);
}
public function testProcessScalarValues(): void
{
$values = ['test string', 42, 3.14, true, false];
- $recursiveProcessor = static fn($v) => $v;
foreach ($values as $value) {
- $result = ($this->processor)($value, $recursiveProcessor);
+ $result = ($this->processor)($value);
$this->assertSame($value, $result);
}
}🤖 Prompt for AI Agents
In tests/Unit/Context/DefaultProcessorTest.php around lines 40 to 56, the test
calls to ($this->processor) pass a second $recursiveProcessor argument but
DefaultProcessor::__invoke() only accepts one parameter; remove the second
argument from both calls so the invocation is ($this->processor)(null) and
($this->processor)($value), and delete or ignore the unused $recursiveProcessor
variable in those tests.
| public function testProcessSimpleArray(): void | ||
| { | ||
| $array = [1, 2, 'three', true]; | ||
| $recursiveProcessor = static fn($v) => $v; // Identity function for simple values | ||
|
|
||
| $result = ($this->processor)($array, $recursiveProcessor); | ||
|
|
||
| $this->assertSame([1, 2, 'three', true], $result); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect method signature in testProcessSimpleArray.
Same issue: line 63 passes two parameters but __invoke() only accepts one.
Apply this diff:
public function testProcessSimpleArray(): void
{
$array = [1, 2, 'three', true];
- $recursiveProcessor = static fn($v) => $v; // Identity function for simple values
-
- $result = ($this->processor)($array, $recursiveProcessor);
+ $result = ($this->processor)($array);
$this->assertSame([1, 2, 'three', true], $result);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testProcessSimpleArray(): void | |
| { | |
| $array = [1, 2, 'three', true]; | |
| $recursiveProcessor = static fn($v) => $v; // Identity function for simple values | |
| $result = ($this->processor)($array, $recursiveProcessor); | |
| $this->assertSame([1, 2, 'three', true], $result); | |
| } | |
| public function testProcessSimpleArray(): void | |
| { | |
| $array = [1, 2, 'three', true]; | |
| $result = ($this->processor)($array); | |
| $this->assertSame([1, 2, 'three', true], $result); | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/Context/DefaultProcessorTest.php around lines 58 to 66, the test
calls ($this->processor)($array, $recursiveProcessor) even though the
processor's __invoke() accepts only one argument; remove the second parameter
and call the processor with a single argument — replace the invocation with
($this->processor)($array) so the test matches the method signature and still
asserts the identity behavior.
| $resource = \fopen('php://memory', 'r'); | ||
|
|
||
| $data = [ | ||
| 'object' => [new \stdClass(), [ | ||
| '@class' => 'stdClass', | ||
| ]], | ||
| 'object with props' => [(object) ['foo' => 'bar'], [ | ||
| '@class' => 'stdClass', | ||
| 'foo' => 'bar', | ||
| ]], | ||
| ]; | ||
|
|
||
| // Close the resource after creating the test data | ||
| \register_shutdown_function(static function () use ($resource): void { | ||
| if (\is_resource($resource)) { | ||
| \fclose($resource); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused resource handling.
The resource created on line 19 is never included in the returned test data array and serves no purpose. The shutdown function registered on lines 32-36 becomes dead code.
Apply this diff to remove the unused code:
public static function allTypesProvider(): array
{
- $resource = \fopen('php://memory', 'r');
-
$data = [
'object' => [new \stdClass(), [
'@class' => 'stdClass',
]],
'object with props' => [(object) ['foo' => 'bar'], [
'@class' => 'stdClass',
'foo' => 'bar',
]],
];
-
- // Close the resource after creating the test data
- \register_shutdown_function(static function () use ($resource): void {
- if (\is_resource($resource)) {
- \fclose($resource);
- }
- });
return $data;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $resource = \fopen('php://memory', 'r'); | |
| $data = [ | |
| 'object' => [new \stdClass(), [ | |
| '@class' => 'stdClass', | |
| ]], | |
| 'object with props' => [(object) ['foo' => 'bar'], [ | |
| '@class' => 'stdClass', | |
| 'foo' => 'bar', | |
| ]], | |
| ]; | |
| // Close the resource after creating the test data | |
| \register_shutdown_function(static function () use ($resource): void { | |
| if (\is_resource($resource)) { | |
| \fclose($resource); | |
| } | |
| }); | |
| public static function allTypesProvider(): array | |
| { | |
| $data = [ | |
| 'object' => [new \stdClass(), [ | |
| '@class' => 'stdClass', | |
| ]], | |
| 'object with props' => [(object) ['foo' => 'bar'], [ | |
| '@class' => 'stdClass', | |
| 'foo' => 'bar', | |
| ]], | |
| ]; | |
| return $data; | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/Context/ObjectProcessor/FallbackProcessorTest.php around lines 19
to 36, remove the unused resource creation and its shutdown handler: the
fopen('php://memory', 'r') stored in $resource is never used in the test data
and the register_shutdown_function that closes it is dead code; delete the
$resource assignment and the register_shutdown_function block so only the $data
array remains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/Context/ObjectProcessor/FallbackProcessor.php (1)
26-33: Guard against infinite recursion when encountering self-references.
After unsetting a self-referential property, the loop immediately reuses the same by-reference variable and feeds the original object back into$processor, so any object referencing itself blows the stack (and the unset never sticks). Iterate without references, skip once removed, and assign back explicitly.- foreach ($result as $k => &$v) { - if ($v === $value) { - unset($result[$k]); - } - - $v = $processor($v); - } + foreach (\array_keys($result) as $k) { + $current = $result[$k]; + if ($current === $value) { + unset($result[$k]); + continue; + } + + $result[$k] = $processor($current); + }src/RpcLogger.php (1)
49-60: Cast the message once before dispatching to logger methods.
AppLogger::error|warning|debug()expect a string. Passing a\Stringablehere will trigger a TypeError, which can happen for perfectly valid PSR-3 callers. Cast the message once up front and reuse it in every branch.- // Process context data for structured logging using the processor manager - $processedContext = ($this->objectProcessor)($context); + // Process context data for structured logging using the processor manager + $processedContext = ($this->objectProcessor)($context); + $message = (string) $message; @@ - PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), - PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), - PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), - 'log' => $this->logger->log((string) $message, $processedContext), - PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), + PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), + PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), + PsrLogLevel::NOTICE, + PsrLogLevel::INFO => $this->logger->info($message, $processedContext), + 'log' => $this->logger->log($message, $processedContext), + PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext),tests/Unit/Context/DefaultProcessorTest.php (4)
11-11: Remove unused import.The import references a non-existent class and is never used.
Apply this diff:
use RoadRunner\PsrLogger\Context\DefaultProcessor; -use RoadRunner\PsrLogger\Context\ObjectProcessor\BuiltInTypeProcessor;
40-45: Fix incorrect method signature.
DefaultProcessor::__invoke()accepts only one parameter(mixed $value), but line 43 passes two parameters. This test will fail.Apply this diff:
public function testProcessNull(): void { - $recursiveProcessor = static fn($v) => $v; - $result = ($this->processor)(null, $recursiveProcessor); + $result = ($this->processor)(null); $this->assertNull($result); }
47-56: Fix incorrect method signature.Same issue: line 53 passes two parameters but
__invoke()only accepts one.Apply this diff:
public function testProcessScalarValues(): void { $values = ['test string', 42, 3.14, true, false]; - $recursiveProcessor = static fn($v) => $v; foreach ($values as $value) { - $result = ($this->processor)($value, $recursiveProcessor); + $result = ($this->processor)($value); $this->assertSame($value, $result); } }
58-66: Fix incorrect method signature.Same issue: line 63 passes two parameters but
__invoke()only accepts one.Apply this diff:
public function testProcessSimpleArray(): void { $array = [1, 2, 'three', true]; - $recursiveProcessor = static fn($v) => $v; // Identity function for simple values - - $result = ($this->processor)($array, $recursiveProcessor); + $result = ($this->processor)($array); $this->assertSame([1, 2, 'three', true], $result); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Context/DefaultProcessor.php(1 hunks)src/Context/ObjectProcessor.php(1 hunks)src/Context/ObjectProcessor/DateTimeProcessor.php(1 hunks)src/Context/ObjectProcessor/FallbackProcessor.php(1 hunks)src/Context/ObjectProcessor/StringableProcessor.php(1 hunks)src/Context/ObjectProcessor/ThrowableProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/Context/DefaultProcessorTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Context/ObjectProcessor.php
- src/Context/DefaultProcessor.php
🧰 Additional context used
🧬 Code graph analysis (6)
src/Context/ObjectProcessor/DateTimeProcessor.php (5)
src/Context/ObjectProcessor/FallbackProcessor.php (2)
Override(17-21)Override(23-36)src/Context/ObjectProcessor/StringableProcessor.php (2)
Override(17-21)Override(23-27)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
Override(18-22)Override(24-35)src/RpcLogger.php (1)
Override(38-63)src/Context/ObjectProcessor.php (2)
canProcess(18-18)process(24-24)
src/RpcLogger.php (5)
src/Context/DefaultProcessor.php (3)
DefaultProcessor(17-73)__construct(22-22)createDefault(29-39)src/Context/ObjectProcessor/DateTimeProcessor.php (2)
Override(20-24)Override(26-30)src/Context/ObjectProcessor/FallbackProcessor.php (2)
Override(17-21)Override(23-36)src/Context/ObjectProcessor/StringableProcessor.php (2)
Override(17-21)Override(23-27)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
Override(18-22)Override(24-35)
tests/Unit/Context/DefaultProcessorTest.php (1)
src/Context/DefaultProcessor.php (2)
DefaultProcessor(17-73)create(24-27)
src/Context/ObjectProcessor/FallbackProcessor.php (2)
src/RpcLogger.php (1)
Override(38-63)src/Context/ObjectProcessor.php (2)
canProcess(18-18)process(24-24)
src/Context/ObjectProcessor/StringableProcessor.php (5)
src/Context/ObjectProcessor/DateTimeProcessor.php (2)
Override(20-24)Override(26-30)src/Context/ObjectProcessor/FallbackProcessor.php (2)
Override(17-21)Override(23-36)src/Context/ObjectProcessor/ThrowableProcessor.php (2)
Override(18-22)Override(24-35)src/RpcLogger.php (1)
Override(38-63)src/Context/ObjectProcessor.php (2)
canProcess(18-18)process(24-24)
src/Context/ObjectProcessor/ThrowableProcessor.php (3)
src/Context/ObjectProcessor/DateTimeProcessor.php (2)
Override(20-24)Override(26-30)src/RpcLogger.php (1)
Override(38-63)src/Context/ObjectProcessor.php (2)
canProcess(18-18)process(24-24)
🪛 GitHub Actions: static analysis
src/RpcLogger.php
[error] 50-50: MixedAssignment: Unable to determine the type that $processedContext is being assigned to
[error] 56-56: MixedArgument: Argument 2 of RoadRunner\Logger\Logger::error cannot be mixed, expecting array<string, mixed>
src/Context/ObjectProcessor/FallbackProcessor.php
[error] 27-27: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 32-32: MixedAssignment: Unable to determine the type that $v is being assigned to
🪛 PHPMD (2.15.0)
src/Context/ObjectProcessor/DateTimeProcessor.php
27-27: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Context/ObjectProcessor/FallbackProcessor.php
18-18: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
src/Context/ObjectProcessor/StringableProcessor.php
24-24: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
src/Context/ObjectProcessor/ThrowableProcessor.php
25-25: Avoid unused parameters such as '$processor'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (5)
src/Context/ObjectProcessor/StringableProcessor.php (2)
17-21: LGTM!The
canProcessimplementation correctly identifiesStringableinstances.
23-27: LGTM! Unused parameter is acceptable.The string cast is correct. The unused
$processorparameter is required by theObjectProcessorinterface but is not needed here since the result is a scalar string (no recursive processing required). This mirrorsDateTimeProcessor, which also doesn't use the parameter.tests/Unit/Context/DefaultProcessorTest.php (3)
18-32: LGTM!The data provider covers a comprehensive range of built-in types including scalars, arrays, null, and resources.
34-38: LGTM!This test correctly uses the data provider and passes a single parameter to the processor.
68-85: LGTM!This test correctly verifies recursive array processing with a single parameter.
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the namespace declaration.
The namespace Context is incomplete and doesn't follow PSR-4 standards. For a test located at tests/Unit/Context/DefaultProcessorTest.php, the namespace should be Tests\Unit\Context.
Apply this diff:
-namespace Context;
+namespace Tests\Unit\Context;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| namespace Context; | |
| namespace Tests\Unit\Context; |
🤖 Prompt for AI Agents
In tests/Unit/Context/DefaultProcessorTest.php around line 5, the namespace is
declared as "Context" which doesn't follow PSR-4 for this test location; change
the namespace declaration to "Tests\Unit\Context" so the class matches the PSR-4
path. Update the namespace line accordingly and ensure any use/imports or class
references within the file remain correct after the namespace change.
| protected function setUp(): void | ||
| { | ||
| $this->processor = DefaultProcessor::create(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use createDefault() to test with the default processor chain.
DefaultProcessor::create() returns an instance with an empty processor list, so object processing (DateTime, Stringable, Throwable, etc.) won't work. Tests should use createDefault() to verify behavior with the standard processor chain.
Apply this diff:
protected function setUp(): void
{
- $this->processor = DefaultProcessor::create();
+ $this->processor = DefaultProcessor::createDefault();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function setUp(): void | |
| { | |
| $this->processor = DefaultProcessor::create(); | |
| } | |
| protected function setUp(): void | |
| { | |
| $this->processor = DefaultProcessor::createDefault(); | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/Context/DefaultProcessorTest.php around lines 87 to 90, the setUp
uses DefaultProcessor::create() which returns an instance with an empty
processor list; update the setup to use DefaultProcessor::createDefault() so the
test runs against the standard processor chain (DateTime, Stringable, Throwable,
etc.) instead of an empty processor list; replace the call in setUp to
createDefault() and keep the rest of the test intact.
I have separated each context type to a processor class for scalable such as
graph TD A[RpcLogger] --> B[ContextProcessorManager] B --> C[NullProcessor] B --> D[ScalarProcessor] B --> E[DateTimeProcessor] B --> F[ThrowableProcessor] B --> G[StringableProcessor] B --> H[ArrayProcessor] B --> I[ResourceProcessor] B --> J[ObjectProcessor] B --> K[FallbackProcessor] B --> L[CustomProcessor] M[Context Data] --> B B --> N[Processed Context] N --> O[RoadRunner Logger]Summary by CodeRabbit
New Features
Tests
Chores